Re: [PATCH v4 bpf-next 3/9] bpf, xdp: extract common XDP program attachment logic

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Maciej Fijalkowski <maciej.fijalkowski@intel.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>,
	David Ahern <dsahern@gmail.com>, Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH v4 bpf-next 3/9] bpf, xdp: extract common XDP program attachment logic
Date: Wed, 22 Jul 2020 12:29:34 -0700	[thread overview]
Message-ID: <CAEf4BzZ-YVaj1rkZGjAe5Fc=rQKUzWCTpiCgRLNhuWmfDXRwUQ@mail.gmail.com> (raw)
In-Reply-To: <20200722191320.GC8874@ranger.igk.intel.com>

On Wed, Jul 22, 2020 at 12:18 PM Maciej Fijalkowski
<maciej.fijalkowski@intel.com> wrote:
>
> On Tue, Jul 21, 2020 at 11:45:56PM -0700, Andrii Nakryiko wrote:
> > Further refactor XDP attachment code. dev_change_xdp_fd() is split into two
> > parts: getting bpf_progs from FDs and attachment logic, working with
> > bpf_progs. This makes attachment  logic a bit more straightforward and
> > prepares code for bpf_xdp_link inclusion, which will share the common logic.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> >  net/core/dev.c | 165 +++++++++++++++++++++++++++----------------------
> >  1 file changed, 91 insertions(+), 74 deletions(-)
> >
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 7e753e248cef..abf573b2dcf4 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -8815,111 +8815,128 @@ static void dev_xdp_uninstall(struct net_device *dev)
> >       }
> >  }
> >
> > -/**
> > - *   dev_change_xdp_fd - set or clear a bpf program for a device rx path
> > - *   @dev: device
> > - *   @extack: netlink extended ack
> > - *   @fd: new program fd or negative value to clear
> > - *   @expected_fd: old program fd that userspace expects to replace or clear
> > - *   @flags: xdp-related flags
> > - *
> > - *   Set or clear a bpf program for a device
> > - */
> > -int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
> > -                   int fd, int expected_fd, u32 flags)
> > +static int dev_xdp_attach(struct net_device *dev, struct netlink_ext_ack *extack,
> > +                       struct bpf_prog *new_prog, struct bpf_prog *old_prog,
> > +                       u32 flags)
> >  {
> > -     const struct net_device_ops *ops = dev->netdev_ops;
> > -     enum bpf_xdp_mode mode = dev_xdp_mode(flags);
> > -     bool offload = mode == XDP_MODE_HW;
> > -     u32 prog_id, expected_id = 0;
> > -     struct bpf_prog *prog;
> > +     struct bpf_prog *cur_prog;
> > +     enum bpf_xdp_mode mode;
> >       bpf_op_t bpf_op;
> >       int err;
> >
> >       ASSERT_RTNL();
>
> couldn't we rely on caller's rtnl assertion? dev_change_xdp_fd() already
> has one.

dev_xdp_attach() is also used from the bpf_link attaching function
(dev_xdp_attach_link() in the later patch). I can remove ASSERT_RTNL()
from dev_change_xdp_fd(), though, it doesn't have to do that check, if
dev_xdp_attach() does it already.

[...]

> > -
> > -             if (prog->expected_attach_type == BPF_XDP_CPUMAP) {
> > -                     NL_SET_ERR_MSG(extack,
> > -                                    "BPF_XDP_CPUMAP programs can not be attached to a device");
> > -                     bpf_prog_put(prog);
> > +             if (new_prog->expected_attach_type == BPF_XDP_CPUMAP) {
> > +                     NL_SET_ERR_MSG(extack, "BPF_XDP_CPUMAP programs can not be attached to a device");
>
> bpf_prog_put() missing?
>

Nope, program putting on error is handled outside the
dev_xdp_attach(), either by bpf() LINK_CREATE handling logic or by
dev_change_xdp_fd().

> >                       return -EINVAL;
> >               }
> > +     }
> >

[...]