From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: David Ahern <dsahern@gmail.com>
Cc: "Andrii Nakryiko" <andriin@fb.com>, bpf <bpf@vger.kernel.org>,
Networking <netdev@vger.kernel.org>,
"Alexei Starovoitov" <ast@fb.com>,
"Daniel Borkmann" <daniel@iogearbox.net>,
"Kernel Team" <kernel-team@fb.com>,
"Jakub Kicinski" <kicinski@fb.com>,
"Andrey Ignatov" <rdna@fb.com>,
"Takshak Chahande" <ctakshak@fb.com>,
"Toke Høiland-Jørgensen" <toke@redhat.com>
Subject: Re: [PATCH bpf-next 2/7] bpf, xdp: add bpf_link-based XDP attachment API
Date: Mon, 13 Jul 2020 15:33:54 -0700 [thread overview]
Message-ID: <CAEf4BzZg2tmNV7LxmcHarZeB+AVmgZW-afzcOUqc7K-5N4NE4Q@mail.gmail.com> (raw)
In-Reply-To: <58dd821b-c9b2-ac45-d47a-e5f75aec3d68@gmail.com>
On Mon, Jul 13, 2020 at 7:19 AM David Ahern <dsahern@gmail.com> wrote:
>
> On 7/10/20 4:49 PM, Andrii Nakryiko wrote:
> > Add bpf_link-based API (bpf_xdp_link) to attach BPF XDP program through
> > BPF_LINK_CREATE command.
> >
> > bpf_xdp_link is mutually exclusive with direct BPF program attachment,
> > previous BPF program should be detached prior to attempting to create a new
> > bpf_xdp_link attachment (for a given XDP mode). Once link is attached, it
> > can't be replaced by other BPF program attachment or link attachment. It will
> > be detached only when the last BPF link FD is closed.
> >
> > bpf_xdp_link will be auto-detached when net_device is shutdown, similarly to
> > how other BPF links behave (cgroup, flow_dissector). At that point bpf_link
> > will become defunct, but won't be destroyed until last FD is closed.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> > include/linux/netdevice.h | 6 +
> > include/uapi/linux/bpf.h | 7 +-
> > kernel/bpf/syscall.c | 5 +
> > net/core/dev.c | 385 ++++++++++++++++++++++++++++----------
>
> That's big diff for 1 patch. A fair bit of is refactoring / code
> movement that can be done in a separate refactoring patch making it
> cleaer what changes you need for the bpf_link piece.
Ok, I'll do another refactoring patch for prog attach logic only.
>
>
> > 4 files changed, 301 insertions(+), 102 deletions(-)
> >
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index d5630e535836..93bcd81d645d 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -886,6 +886,7 @@ struct bpf_prog_offload_ops;
> > struct netlink_ext_ack;
> > struct xdp_umem;
> > struct xdp_dev_bulk_queue;
> > +struct bpf_xdp_link;
> >
> > enum bpf_xdp_mode {
> > XDP_MODE_SKB = 0,
> > @@ -896,6 +897,7 @@ enum bpf_xdp_mode {
> >
> > struct bpf_xdp_entity {
> > struct bpf_prog *prog;
> > + struct bpf_xdp_link *link;
> > };
> >
> > struct netdev_bpf {
> > @@ -3824,6 +3826,10 @@ typedef int (*bpf_op_t)(struct net_device *dev, struct netdev_bpf *bpf);
> > int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
> > int fd, int expected_fd, u32 flags);
> > u32 dev_xdp_prog_id(struct net_device *dev, enum bpf_xdp_mode mode);
> > +
> > +struct bpf_xdp_link;
>
> already stated above.
oh, right, will drop
>
> > +int bpf_xdp_link_attach(const union bpf_attr *attr, struct bpf_prog *prog);
> > +
> > int xdp_umem_query(struct net_device *dev, u16 queue_id);
> >
> > int __dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 548a749aebb3..41eba148217b 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -227,6 +227,7 @@ enum bpf_attach_type {
> > BPF_CGROUP_INET6_GETSOCKNAME,
> > BPF_XDP_DEVMAP,
> > BPF_CGROUP_INET_SOCK_RELEASE,
> > + BPF_XDP,
>
> This really does not add value for the uapi. The link_type uniquely
> identifies the type and the expected program type.
Yes, but that's how PROG_ATTACH/LINK_CREATE is set up. We had a
similar discussion for SK_LOOKUP recently. The only downside right now
is increasing struct cgroup_bpf size, but I think we have a plan for
mitigating it, similarly how netns bpf_link does it.
>
> > __MAX_BPF_ATTACH_TYPE
> > };
> >
> > @@ -239,6 +240,7 @@ enum bpf_link_type {
> > BPF_LINK_TYPE_CGROUP = 3,
> > BPF_LINK_TYPE_ITER = 4,
> > BPF_LINK_TYPE_NETNS = 5,
> > + BPF_LINK_TYPE_XDP = 6,
> >
> > MAX_BPF_LINK_TYPE,
> > };
[...]
> > + /* Drivers assume refcnt is already incremented (i.e, prog pointer is
> > + * "moved" into driver), so they don't increment it on their own, but
> > + * they do decrement refcnt when program is detached or replaced.
> > + * Given net_device also owns link/prog, we need to bump refcnt here
> > + * to prevent drivers from underflowing it.
> > + */
> > + if (prog)
> > + bpf_prog_inc(prog);
>
> Why is this refcnt bump not needed today but is needed for your change?
Previously driver/generic_xdp_install "owned" this program and assumed
an already incremented ref_cnt, but dropped that count when the
current program was replaced with a new one (or NULL). Now, net_device
*also* owns prog (in xdp_state[mode].prog), in addition to whatever
book-keeping driver does internally. So there needs to be extra
refcnt.
But you are right that this should have been part of refactoring in
patch #1, I'll move it there.
>
> > err = bpf_op(dev, &xdp);
> > if (err)
> > return err;
>
> and the error path is not decrementing it.
yep, great catch, thanks!
>
> > @@ -8756,39 +8811,221 @@ static int dev_xdp_install(struct net_device *dev, enum bpf_xdp_mode mode,
> >
[...]
> >
> > - if (dev_xdp_prog_id(dev, XDP_MODE_DRV)) {
> > - WARN_ON(dev_xdp_install(dev, XDP_MODE_DRV, ndo_bpf,
> > - NULL, 0, NULL));
> > - dev_xdp_set_prog(dev, XDP_MODE_DRV, NULL);
> > + /* link supports only XDP mode flags */
> > + if (link && (flags & ~XDP_FLAGS_MODES))
> > + return -EINVAL;
>
> everyone of the -errno returns needs an extack message explaining why
sure
next prev parent reply other threads:[~2020-07-13 22:34 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20200710224924.4087399-1-andriin@fb.com>
[not found] ` <20200710224924.4087399-3-andriin@fb.com>
2020-07-13 14:19 ` [PATCH bpf-next 2/7] bpf, xdp: add bpf_link-based XDP attachment API David Ahern
2020-07-13 22:33 ` Andrii Nakryiko [this message]
2020-07-14 13:57 ` Toke Høiland-Jørgensen
2020-07-14 18:59 ` Andrii Nakryiko
2020-07-14 20:12 ` Toke Høiland-Jørgensen
2020-07-14 20:37 ` Andrii Nakryiko
2020-07-14 21:41 ` Toke Høiland-Jørgensen
2020-07-14 22:26 ` Andrii Nakryiko
2020-07-15 15:48 ` Toke Høiland-Jørgensen
2020-07-15 20:54 ` Andrii Nakryiko
2020-07-16 10:52 ` Toke Høiland-Jørgensen
2020-07-22 6:45 ` Andrii Nakryiko
[not found] ` <20200710224924.4087399-5-andriin@fb.com>
2020-07-13 14:32 ` [PATCH bpf-next 4/7] bpf: implement BPF XDP link-specific introspection APIs David Ahern
2020-07-13 22:41 ` Andrii Nakryiko
2020-07-13 5:12 [PATCH bpf-next 0/7] BPF XDP link Andrii Nakryiko
2020-07-13 5:12 ` [PATCH bpf-next 2/7] bpf, xdp: add bpf_link-based XDP attachment API Andrii Nakryiko
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=CAEf4BzZg2tmNV7LxmcHarZeB+AVmgZW-afzcOUqc7K-5N4NE4Q@mail.gmail.com \
--to=andrii.nakryiko@gmail.com \
--cc=andriin@fb.com \
--cc=ast@fb.com \
--cc=bpf@vger.kernel.org \
--cc=ctakshak@fb.com \
--cc=daniel@iogearbox.net \
--cc=dsahern@gmail.com \
--cc=kernel-team@fb.com \
--cc=kicinski@fb.com \
--cc=netdev@vger.kernel.org \
--cc=rdna@fb.com \
--cc=toke@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
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;
as well as URLs for NNTP newsgroup(s).