From: Daniel Borkmann <daniel@iogearbox•net>
To: Lawrence Brakmo <brakmo@fb•com>, netdev <netdev@vger•kernel.org>
Cc: Kernel Team <Kernel-team@fb•com>, Blake Matheny <bmatheny@fb•com>,
Alexei Starovoitov <ast@fb•com>,
David Ahern <dsa@cumulusnetworks•com>
Subject: Re: [RFC PATCH net-next v2 01/15] bpf: BPF support for socket ops
Date: Mon, 19 Jun 2017 20:44:02 +0200 [thread overview]
Message-ID: <59481B72.2050307@iogearbox.net> (raw)
In-Reply-To: <1274D98B-287D-492C-88CC-A14617EB8AAC@fb.com>
On 06/17/2017 01:41 AM, Lawrence Brakmo wrote:
> On 6/16/17, 5:07 AM, "Daniel Borkmann" <daniel@iogearbox•net> wrote:
[...]
> I see. You are saying have one struct in common but still keep the two
> PROG_TYPES? That makes sense. Do we really need two different
> is_valid_access functions? Both types should be able to see all
> the fields (otherwise adding new fields becomes messy).
Would probably leave the two is_valid_access() separate initially, and
once people ask for it we could potentially open this up to some of
the other fields that are available at that time.
> > Currently there are two types of ops. The first type expects the BPF
> > program to return a value which is then used by the caller (or a
> > negative value to indicate the operation is not supported). The second
> > type expects state changes to be done by the BPF program, for example
> > through a setsockopt BPF helper function, and they ignore the return
> > value.
[...]
> > +/* Call BPF_SOCKET_OPS program that returns an int. If the return value
> > + * is < 0, then the BPF op failed (for example if the loaded BPF
> > + * program does not support the chosen operation or there is no BPF
> > + * program loaded).
> > + */
> > +#ifdef CONFIG_BPF
> > +static inline int tcp_call_bpf(struct sock *sk, bool is_req_sock, int op)
> > +{
> > + struct bpf_socket_ops_kern socket_ops;
> > +
> > + memset(&socket_ops, 0, sizeof(socket_ops));
> > + socket_ops.sk = sk;
> > + socket_ops.is_req_sock = is_req_sock ? 1 : 0;
>
> Is is_req_sock actually used here in this patch (apart from setting it)?
> Not seeing that BPF prog will access it, if it also shouldn't access it,
> then bool type would be better.
>
> The only reason I used a bit was in case I wanted to add more fields later on.
> Does it make sense or should I just use bool?
Didn't know that, but I think starting out with bool seems a bit
cleaner, if needed we could later still switch to bitfield.
> > + socket_ops.op = op;
> > +
> > + return bpf_socket_ops_call(&socket_ops);
> > +}
[...]
> > +/* Global BPF program for sockets */
> > +static struct bpf_prog *bpf_socket_ops_prog;
> > +static DEFINE_RWLOCK(bpf_socket_ops_lock);
> > +
> > +int bpf_socket_ops_set_prog(int fd)
> > +{
> > + int err = 0;
> > +
> > + write_lock(&bpf_socket_ops_lock);
> > + if (bpf_socket_ops_prog) {
> > + bpf_prog_put(bpf_socket_ops_prog);
> > + bpf_socket_ops_prog = NULL;
> > + }
> > +
> > + /* fd of zero is used as a signal to remove the current
> > + * bpf_socket_ops_prog.
> > + */
> > + if (fd == 0) {
>
> Can we make the fd related semantics similar to dev_change_xdp_fd()?
>
> Do you mean remove program is fd < 0 instead of == 0?
Yes, that and also the ordering of dropping the ref of the existing
bpf_socket_ops_prog program with setting the new one, so you can
convert bpf_socket_ops_prog to RCU more easily.
> > + write_unlock(&bpf_socket_ops_lock);
> > + return 1;
> > + }
> > +
> > + bpf_socket_ops_prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_SOCKET_OPS);
> > + if (IS_ERR(bpf_socket_ops_prog)) {
> > + bpf_prog_put(bpf_socket_ops_prog);
>
> This will crash the kernel, passing err value to bpf_prog_put().
[...]
next prev parent reply other threads:[~2017-06-19 18:44 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-06-15 20:08 [RFC PATCH net-next v2 00/15] bpf: BPF support for socket ops Lawrence Brakmo
2017-06-15 20:08 ` [RFC PATCH net-next v2 01/15] " Lawrence Brakmo
2017-06-16 12:07 ` Daniel Borkmann
2017-06-16 23:41 ` Lawrence Brakmo
2017-06-19 18:44 ` Daniel Borkmann [this message]
2017-06-19 20:49 ` Lawrence Brakmo
2017-06-17 21:48 ` Lawrence Brakmo
2017-06-19 18:52 ` Daniel Borkmann
2017-06-19 20:49 ` Lawrence Brakmo
2017-06-15 20:08 ` [RFC PATCH net-next v2 02/15] bpf: program to load socketops BPF programs Lawrence Brakmo
2017-06-15 20:08 ` [RFC PATCH net-next v2 03/15] bpf: Support for per connection SYN/SYN-ACK RTOs Lawrence Brakmo
2017-06-15 20:08 ` [RFC PATCH net-next v2 04/15] bpf: Sample bpf program to set " Lawrence Brakmo
2017-06-15 20:08 ` [RFC PATCH net-next v2 05/15] bpf: Support for setting initial receive window Lawrence Brakmo
2017-06-15 20:08 ` [RFC PATCH net-next v2 06/15] bpf: Sample bpf program to set initial window Lawrence Brakmo
2017-06-15 20:08 ` [RFC PATCH net-next v2 07/15] bpf: Add setsockopt helper function to bpf Lawrence Brakmo
2017-06-16 13:27 ` Daniel Borkmann
2017-06-17 23:17 ` Lawrence Brakmo
2017-06-15 20:08 ` [RFC PATCH net-next v2 08/15] bpf: Add TCP connection BPF callbacks Lawrence Brakmo
2017-06-15 20:08 ` [RFC PATCH net-next v2 09/15] bpf: Sample BPF program to set buffer sizes Lawrence Brakmo
2017-06-15 20:08 ` [RFC PATCH net-next v2 10/15] bpf: Add support for changing congestion control Lawrence Brakmo
2017-06-16 13:58 ` Daniel Borkmann
2017-06-18 2:39 ` Lawrence Brakmo
2017-06-19 22:34 ` Daniel Borkmann
2017-06-20 0:35 ` Lawrence Brakmo
2017-06-15 20:08 ` [RFC PATCH net-next v2 11/15] bpf: Sample BPF program to set " Lawrence Brakmo
2017-06-15 20:08 ` [RFC PATCH net-next v2 12/15] bpf: Adds support for setting initial cwnd Lawrence Brakmo
2017-06-15 20:08 ` [RFC PATCH net-next v2 13/15] bpf: Sample BPF program to set " Lawrence Brakmo
2017-06-15 20:08 ` [RFC PATCH net-next v2 14/15] bpf: Adds support for setting sndcwnd clamp Lawrence Brakmo
2017-06-15 20:08 ` [RFC PATCH net-next v2 15/15] bpf: Sample bpf program to set " Lawrence Brakmo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=59481B72.2050307@iogearbox.net \
--to=daniel@iogearbox$(echo .)net \
--cc=Kernel-team@fb$(echo .)com \
--cc=ast@fb$(echo .)com \
--cc=bmatheny@fb$(echo .)com \
--cc=brakmo@fb$(echo .)com \
--cc=dsa@cumulusnetworks$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox