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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 22 Jul 2020 12:29:34 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Maciej Fijalkowski <maciej.fijalkowski@...el.com>
Cc:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        David Ahern <dsahern@...il.com>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v4 bpf-next 3/9] bpf, xdp: extract common XDP program
 attachment logic

On Wed, Jul 22, 2020 at 12:18 PM Maciej Fijalkowski
<maciej.fijalkowski@...el.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@...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;
> >               }
> > +     }
> >

[...]