On Mon, Jul 13, 2020 at 7:19 AM David Ahern <dsahern@xxxxxxxxx> 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@xxxxxx>
> > ---
> > 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
