| Message ID | 20200722064603.3350758-4-andriin@fb.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | BPF XDP link | expand |
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. > > - bpf_op = dev_xdp_bpf_op(dev, mode); > - if (!bpf_op) { > - NL_SET_ERR_MSG(extack, "underlying driver does not support XDP in native mode"); > - return -EOPNOTSUPP; > + /* just one XDP mode bit should be set, zero defaults to SKB mode */ > + if (hweight32(flags & XDP_FLAGS_MODES) > 1) { > + NL_SET_ERR_MSG(extack, "Only one XDP mode flag can be set"); > + return -EINVAL; > + } > + /* old_prog != NULL implies XDP_FLAGS_REPLACE is set */ > + if (old_prog && !(flags & XDP_FLAGS_REPLACE)) { > + NL_SET_ERR_MSG(extack, "XDP_FLAGS_REPLACE is not specified"); > + return -EINVAL; > } > > - prog_id = dev_xdp_prog_id(dev, mode); > - if (flags & XDP_FLAGS_REPLACE) { > - if (expected_fd >= 0) { > - prog = bpf_prog_get_type_dev(expected_fd, > - BPF_PROG_TYPE_XDP, > - bpf_op == ops->ndo_bpf); > - if (IS_ERR(prog)) > - return PTR_ERR(prog); > - expected_id = prog->aux->id; > - bpf_prog_put(prog); > - } > - > - if (prog_id != expected_id) { > - NL_SET_ERR_MSG(extack, "Active program does not match expected"); > - return -EEXIST; > - } > + mode = dev_xdp_mode(flags); > + cur_prog = dev_xdp_prog(dev, mode); > + if ((flags & XDP_FLAGS_REPLACE) && cur_prog != old_prog) { > + NL_SET_ERR_MSG(extack, "Active program does not match expected"); > + return -EEXIST; > } > - if (fd >= 0) { > + if ((flags & XDP_FLAGS_UPDATE_IF_NOEXIST) && cur_prog) { > + NL_SET_ERR_MSG(extack, "XDP program already attached"); > + return -EBUSY; > + } > + > + if (new_prog) { > + bool offload = mode == XDP_MODE_HW; > enum bpf_xdp_mode other_mode = mode == XDP_MODE_SKB > ? XDP_MODE_DRV : XDP_MODE_SKB; > > - if (!offload && dev_xdp_prog_id(dev, other_mode)) { > + if (!offload && dev_xdp_prog(dev, other_mode)) { > NL_SET_ERR_MSG(extack, "Native and generic XDP can't be active at the same time"); > return -EEXIST; > } > - > - if ((flags & XDP_FLAGS_UPDATE_IF_NOEXIST) && prog_id) { > - NL_SET_ERR_MSG(extack, "XDP program already attached"); > - return -EBUSY; > - } > - > - prog = bpf_prog_get_type_dev(fd, BPF_PROG_TYPE_XDP, > - bpf_op == ops->ndo_bpf); > - if (IS_ERR(prog)) > - return PTR_ERR(prog); > - > - if (!offload && bpf_prog_is_dev_bound(prog->aux)) { > + if (!offload && bpf_prog_is_dev_bound(new_prog->aux)) { > NL_SET_ERR_MSG(extack, "Using device-bound program without HW_MODE flag is not supported"); > - bpf_prog_put(prog); > return -EINVAL; > } > - > - if (prog->expected_attach_type == BPF_XDP_DEVMAP) { > + if (new_prog->expected_attach_type == BPF_XDP_DEVMAP) { > NL_SET_ERR_MSG(extack, "BPF_XDP_DEVMAP programs can not be attached to a device"); > - bpf_prog_put(prog); > return -EINVAL; > } > - > - 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? > return -EINVAL; > } > + } > > - /* prog->aux->id may be 0 for orphaned device-bound progs */ > - if (prog->aux->id && prog->aux->id == prog_id) { > - bpf_prog_put(prog); > - return 0; > + /* don't call drivers if the effective program didn't change */ > + if (new_prog != cur_prog) { > + bpf_op = dev_xdp_bpf_op(dev, mode); > + if (!bpf_op) { > + NL_SET_ERR_MSG(extack, "Underlying driver does not support XDP in native mode"); > + return -EOPNOTSUPP; > } > - } else { > - if (!prog_id) > - return 0; > - prog = NULL; > - } > > - err = dev_xdp_install(dev, mode, bpf_op, extack, flags, prog); > - if (err < 0 && prog) { > - bpf_prog_put(prog); > - return err; > + err = dev_xdp_install(dev, mode, bpf_op, extack, flags, new_prog); > + if (err) > + return err; > } > - dev_xdp_set_prog(dev, mode, prog); > + > + dev_xdp_set_prog(dev, mode, new_prog); > + if (cur_prog) > + bpf_prog_put(cur_prog); > > return 0; > } > > +/** > + * 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) > +{ > + enum bpf_xdp_mode mode = dev_xdp_mode(flags); > + struct bpf_prog *new_prog = NULL, *old_prog = NULL; > + int err; > + > + ASSERT_RTNL(); > + > + if (fd >= 0) { > + new_prog = bpf_prog_get_type_dev(fd, BPF_PROG_TYPE_XDP, > + mode != XDP_MODE_SKB); > + if (IS_ERR(new_prog)) > + return PTR_ERR(new_prog); > + } > + > + if (expected_fd >= 0) { > + old_prog = bpf_prog_get_type_dev(expected_fd, BPF_PROG_TYPE_XDP, > + mode != XDP_MODE_SKB); > + if (IS_ERR(old_prog)) { > + err = PTR_ERR(old_prog); > + old_prog = NULL; > + goto err_out; > + } > + } > + > + err = dev_xdp_attach(dev, extack, new_prog, old_prog, flags); > + > +err_out: > + if (err && new_prog) > + bpf_prog_put(new_prog); > + if (old_prog) > + bpf_prog_put(old_prog); > + return err; > +} > + > /** > * dev_new_index - allocate an ifindex > * @net: the applicable net namespace > -- > 2.24.1 >
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; > > } > > + } > > [...]
Andrii Nakryiko <andriin@fb.com> writes: > 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(); > > - bpf_op = dev_xdp_bpf_op(dev, mode); > - if (!bpf_op) { > - NL_SET_ERR_MSG(extack, "underlying driver does not > support XDP in native mode"); > - return -EOPNOTSUPP; > + /* just one XDP mode bit should be set, zero defaults to > SKB mode */ > + if (hweight32(flags & XDP_FLAGS_MODES) > 1) { Not sure if it's more efficient but running if ((flags & XDP) & ((flags & XDP) - 1) != 0) returns whether a number is a multiple of 2. Should be equivalent to what you checked with hweight32. It is less readable though. Just thought I'd throw that in. Taken from https://graphics.stanford.edu/~seander/bithacks.html#DetermineIfPowerOf2 > + NL_SET_ERR_MSG(extack, "Only one XDP mode flag can > be set"); > + return -EINVAL; > + } > + /* old_prog != NULL implies XDP_FLAGS_REPLACE is set */ > + if (old_prog && !(flags & XDP_FLAGS_REPLACE)) { > + NL_SET_ERR_MSG(extack, "XDP_FLAGS_REPLACE is not > specified"); > + return -EINVAL; > } > > - prog_id = dev_xdp_prog_id(dev, mode); > - if (flags & XDP_FLAGS_REPLACE) { > - if (expected_fd >= 0) { > - prog = bpf_prog_get_type_dev(expected_fd, > - > BPF_PROG_TYPE_XDP, > - bpf_op == > ops->ndo_bpf); > - if (IS_ERR(prog)) > - return PTR_ERR(prog); > - expected_id = prog->aux->id; > - bpf_prog_put(prog); > - } > - > - if (prog_id != expected_id) { > - NL_SET_ERR_MSG(extack, "Active program > does not match expected"); > - return -EEXIST; > - } > + mode = dev_xdp_mode(flags); > + cur_prog = dev_xdp_prog(dev, mode); > + if ((flags & XDP_FLAGS_REPLACE) && cur_prog != old_prog) { > + NL_SET_ERR_MSG(extack, "Active program does not > match expected"); > + return -EEXIST; > } > - if (fd >= 0) { > + if ((flags & XDP_FLAGS_UPDATE_IF_NOEXIST) && cur_prog) { > + NL_SET_ERR_MSG(extack, "XDP program already > attached"); > + return -EBUSY; > + } > + > + if (new_prog) { > + bool offload = mode == XDP_MODE_HW; > enum bpf_xdp_mode other_mode = mode == > XDP_MODE_SKB > ? XDP_MODE_DRV : > XDP_MODE_SKB; > > - if (!offload && dev_xdp_prog_id(dev, other_mode)) > { > + if (!offload && dev_xdp_prog(dev, other_mode)) { > NL_SET_ERR_MSG(extack, "Native and generic > XDP can't be active at the same time"); > return -EEXIST; > } > - > - if ((flags & XDP_FLAGS_UPDATE_IF_NOEXIST) && > prog_id) { > - NL_SET_ERR_MSG(extack, "XDP program > already attached"); > - return -EBUSY; > - } > - > - prog = bpf_prog_get_type_dev(fd, > BPF_PROG_TYPE_XDP, > - bpf_op == > ops->ndo_bpf); > - if (IS_ERR(prog)) > - return PTR_ERR(prog); > - > - if (!offload && bpf_prog_is_dev_bound(prog->aux)) > { > + if (!offload && > bpf_prog_is_dev_bound(new_prog->aux)) { > NL_SET_ERR_MSG(extack, "Using device-bound > program without HW_MODE flag is not supported"); > - bpf_prog_put(prog); > return -EINVAL; > } > - > - if (prog->expected_attach_type == BPF_XDP_DEVMAP) > { > + if (new_prog->expected_attach_type == > BPF_XDP_DEVMAP) { > NL_SET_ERR_MSG(extack, "BPF_XDP_DEVMAP > programs can not be attached to a device"); > - bpf_prog_put(prog); > return -EINVAL; > } > - > - 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"); > return -EINVAL; > } > + } > > - /* prog->aux->id may be 0 for orphaned > device-bound progs */ > - if (prog->aux->id && prog->aux->id == prog_id) { > - bpf_prog_put(prog); > - return 0; > + /* don't call drivers if the effective program didn't > change */ > + if (new_prog != cur_prog) { > + bpf_op = dev_xdp_bpf_op(dev, mode); > + if (!bpf_op) { > + NL_SET_ERR_MSG(extack, "Underlying driver > does not support XDP in native mode"); > + return -EOPNOTSUPP; > } > - } else { > - if (!prog_id) > - return 0; > - prog = NULL; > - } > > - err = dev_xdp_install(dev, mode, bpf_op, extack, flags, > prog); > - if (err < 0 && prog) { > - bpf_prog_put(prog); > - return err; > + err = dev_xdp_install(dev, mode, bpf_op, extack, > flags, new_prog); > + if (err) > + return err; > } > - dev_xdp_set_prog(dev, mode, prog); > + > + dev_xdp_set_prog(dev, mode, new_prog); > + if (cur_prog) > + bpf_prog_put(cur_prog); > > return 0; > } > > +/** > + * 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) > +{ > + enum bpf_xdp_mode mode = dev_xdp_mode(flags); > + struct bpf_prog *new_prog = NULL, *old_prog = NULL; > + int err; > + > + ASSERT_RTNL(); > + > + if (fd >= 0) { > + new_prog = bpf_prog_get_type_dev(fd, > BPF_PROG_TYPE_XDP, > + mode != > XDP_MODE_SKB); > + if (IS_ERR(new_prog)) > + return PTR_ERR(new_prog); > + } > + > + if (expected_fd >= 0) { > + old_prog = bpf_prog_get_type_dev(expected_fd, > BPF_PROG_TYPE_XDP, > + mode != > XDP_MODE_SKB); > + if (IS_ERR(old_prog)) { > + err = PTR_ERR(old_prog); > + old_prog = NULL; > + goto err_out; > + } > + } > + > + err = dev_xdp_attach(dev, extack, new_prog, old_prog, > flags); > + > +err_out: > + if (err && new_prog) > + bpf_prog_put(new_prog); > + if (old_prog) > + bpf_prog_put(old_prog); > + return err; > +} > + > /** > * dev_new_index - allocate an ifindex > * @net: the applicable net namespace
On Mon, Jul 27, 2020 at 5:08 AM Shay Agroskin <shayagr@amazon.com> wrote: > > > Andrii Nakryiko <andriin@fb.com> writes: > > > 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(); > > > > - bpf_op = dev_xdp_bpf_op(dev, mode); > > - if (!bpf_op) { > > - NL_SET_ERR_MSG(extack, "underlying driver does not > > support XDP in native mode"); > > - return -EOPNOTSUPP; > > + /* just one XDP mode bit should be set, zero defaults to > > SKB mode */ > > + if (hweight32(flags & XDP_FLAGS_MODES) > 1) { > > Not sure if it's more efficient but running > if ((flags & XDP) & ((flags & XDP) - 1) != 0) > > returns whether a number is a multiple of 2. > Should be equivalent to what you checked with hweight32. It is > less readable though. Just thought I'd throw that in. so I just preserved what is there in netlink-handling code. It also is not a performance-critical part. What you propose might work, but using hweight32 is more explicit about allowing zero or one bits set. > Taken from > https://graphics.stanford.edu/~seander/bithacks.html#DetermineIfPowerOf2 > > > + NL_SET_ERR_MSG(extack, "Only one XDP mode flag can > > be set"); > > + return -EINVAL; > > + } > > + /* old_prog != NULL implies XDP_FLAGS_REPLACE is set */ > > + if (old_prog && !(flags & XDP_FLAGS_REPLACE)) { > > + NL_SET_ERR_MSG(extack, "XDP_FLAGS_REPLACE is not > > specified"); > > + return -EINVAL; > > } > > [...]
On 07/21, 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. It looks like this patch breaks xdp tests for me: * test_xdping.sh * test_xdp_vlan.sh Can you please verify on your side? Looking at tools/testing/selftests/bpf/xdping.c I see it has: static __u32 xdp_flags = XDP_FLAGS_UPDATE_IF_NOEXIST; And it attaches program two times in the same net namespace, so I don't see how it could've worked before the change :-/ (unless, of coarse, the previous code was buggy).
On Tue, Aug 11, 2020 at 11:14 AM <sdf@google.com> wrote: > > On 07/21, 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. > It looks like this patch breaks xdp tests for me: > * test_xdping.sh > * test_xdp_vlan.sh > > Can you please verify on your side? > > Looking at tools/testing/selftests/bpf/xdping.c I see it has: > static __u32 xdp_flags = XDP_FLAGS_UPDATE_IF_NOEXIST; > > And it attaches program two times in the same net namespace, > so I don't see how it could've worked before the change :-/ > (unless, of coarse, the previous code was buggy). Ok, so according to the old logic, XDP_FLAGS_UPDATE_IF_NOEXIST flag is only checked if new program fd is not -1. So if we are installing a new program and specify XDP_FLAGS_UPDATE_IF_NOEXIST, we'll be allowed to do this only if there is no BPF program already attached. But we are uninstalling program, then XDP_FLAGS_UPDATE_IF_NOEXIST is ignored and we are allowed to uninstall any BPF program. I can easily fix this by moving the XDP_FLAGS_UPDATE_IF_NOEXIST check inside `if (new_prog) {}` section. I'm not sure which semantics was actually originally intended. Maybe XDP folks can chime in here?
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(); - bpf_op = dev_xdp_bpf_op(dev, mode); - if (!bpf_op) { - NL_SET_ERR_MSG(extack, "underlying driver does not support XDP in native mode"); - return -EOPNOTSUPP; + /* just one XDP mode bit should be set, zero defaults to SKB mode */ + if (hweight32(flags & XDP_FLAGS_MODES) > 1) { + NL_SET_ERR_MSG(extack, "Only one XDP mode flag can be set"); + return -EINVAL; + } + /* old_prog != NULL implies XDP_FLAGS_REPLACE is set */ + if (old_prog && !(flags & XDP_FLAGS_REPLACE)) { + NL_SET_ERR_MSG(extack, "XDP_FLAGS_REPLACE is not specified"); + return -EINVAL; } - prog_id = dev_xdp_prog_id(dev, mode); - if (flags & XDP_FLAGS_REPLACE) { - if (expected_fd >= 0) { - prog = bpf_prog_get_type_dev(expected_fd, - BPF_PROG_TYPE_XDP, - bpf_op == ops->ndo_bpf); - if (IS_ERR(prog)) - return PTR_ERR(prog); - expected_id = prog->aux->id; - bpf_prog_put(prog); - } - - if (prog_id != expected_id) { - NL_SET_ERR_MSG(extack, "Active program does not match expected"); - return -EEXIST; - } + mode = dev_xdp_mode(flags); + cur_prog = dev_xdp_prog(dev, mode); + if ((flags & XDP_FLAGS_REPLACE) && cur_prog != old_prog) { + NL_SET_ERR_MSG(extack, "Active program does not match expected"); + return -EEXIST; } - if (fd >= 0) { + if ((flags & XDP_FLAGS_UPDATE_IF_NOEXIST) && cur_prog) { + NL_SET_ERR_MSG(extack, "XDP program already attached"); + return -EBUSY; + } + + if (new_prog) { + bool offload = mode == XDP_MODE_HW; enum bpf_xdp_mode other_mode = mode == XDP_MODE_SKB ? XDP_MODE_DRV : XDP_MODE_SKB; - if (!offload && dev_xdp_prog_id(dev, other_mode)) { + if (!offload && dev_xdp_prog(dev, other_mode)) { NL_SET_ERR_MSG(extack, "Native and generic XDP can't be active at the same time"); return -EEXIST; } - - if ((flags & XDP_FLAGS_UPDATE_IF_NOEXIST) && prog_id) { - NL_SET_ERR_MSG(extack, "XDP program already attached"); - return -EBUSY; - } - - prog = bpf_prog_get_type_dev(fd, BPF_PROG_TYPE_XDP, - bpf_op == ops->ndo_bpf); - if (IS_ERR(prog)) - return PTR_ERR(prog); - - if (!offload && bpf_prog_is_dev_bound(prog->aux)) { + if (!offload && bpf_prog_is_dev_bound(new_prog->aux)) { NL_SET_ERR_MSG(extack, "Using device-bound program without HW_MODE flag is not supported"); - bpf_prog_put(prog); return -EINVAL; } - - if (prog->expected_attach_type == BPF_XDP_DEVMAP) { + if (new_prog->expected_attach_type == BPF_XDP_DEVMAP) { NL_SET_ERR_MSG(extack, "BPF_XDP_DEVMAP programs can not be attached to a device"); - bpf_prog_put(prog); return -EINVAL; } - - 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"); return -EINVAL; } + } - /* prog->aux->id may be 0 for orphaned device-bound progs */ - if (prog->aux->id && prog->aux->id == prog_id) { - bpf_prog_put(prog); - return 0; + /* don't call drivers if the effective program didn't change */ + if (new_prog != cur_prog) { + bpf_op = dev_xdp_bpf_op(dev, mode); + if (!bpf_op) { + NL_SET_ERR_MSG(extack, "Underlying driver does not support XDP in native mode"); + return -EOPNOTSUPP; } - } else { - if (!prog_id) - return 0; - prog = NULL; - } - err = dev_xdp_install(dev, mode, bpf_op, extack, flags, prog); - if (err < 0 && prog) { - bpf_prog_put(prog); - return err; + err = dev_xdp_install(dev, mode, bpf_op, extack, flags, new_prog); + if (err) + return err; } - dev_xdp_set_prog(dev, mode, prog); + + dev_xdp_set_prog(dev, mode, new_prog); + if (cur_prog) + bpf_prog_put(cur_prog); return 0; } +/** + * 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) +{ + enum bpf_xdp_mode mode = dev_xdp_mode(flags); + struct bpf_prog *new_prog = NULL, *old_prog = NULL; + int err; + + ASSERT_RTNL(); + + if (fd >= 0) { + new_prog = bpf_prog_get_type_dev(fd, BPF_PROG_TYPE_XDP, + mode != XDP_MODE_SKB); + if (IS_ERR(new_prog)) + return PTR_ERR(new_prog); + } + + if (expected_fd >= 0) { + old_prog = bpf_prog_get_type_dev(expected_fd, BPF_PROG_TYPE_XDP, + mode != XDP_MODE_SKB); + if (IS_ERR(old_prog)) { + err = PTR_ERR(old_prog); + old_prog = NULL; + goto err_out; + } + } + + err = dev_xdp_attach(dev, extack, new_prog, old_prog, flags); + +err_out: + if (err && new_prog) + bpf_prog_put(new_prog); + if (old_prog) + bpf_prog_put(old_prog); + return err; +} + /** * dev_new_index - allocate an ifindex * @net: the applicable net namespace
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(-)