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