On Tue, Dec 13, 2022 at 3:25 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>
> On 12/12/22 6:35 PM, Stanislav Fomichev wrote:
> > New flag BPF_F_XDP_DEV_BOUND_ONLY plus all the infra to have a way
> > to associate a netdev with a BPF program at load time.
> >
> > Some existing 'offloaded' routines are renamed to 'dev_bound' for
> > consistency with the rest.
> >
> > Also moved a bunch of code around to avoid forward declarations.
>
> There are too many things in one patch. It becomes quite hard to follow, eg. I
> have to go back-and-forth a few times within this patch to confirm what change
> is just a move. Please put the "moved a bunch of code around to avoid forward
> declarations" in one individual patch and also the
> "late_initcall(bpf_offload_init)" change in another individual patch.
Ugh, sorry, good point will definitely split more :-(
> [ ... ]
>
> > -int bpf_prog_offload_init(struct bpf_prog *prog, union bpf_attr *attr)
> > +static int __bpf_offload_dev_netdev_register(struct bpf_offload_dev *offdev,
> > + struct net_device *netdev)
> > +{
> > + struct bpf_offload_netdev *ondev;
> > + int err;
> > +
> > + ondev = kzalloc(sizeof(*ondev), GFP_KERNEL);
> > + if (!ondev)
> > + return -ENOMEM;
> > +
> > + ondev->netdev = netdev;
> > + ondev->offdev = offdev;
> > + INIT_LIST_HEAD(&ondev->progs);
> > + INIT_LIST_HEAD(&ondev->maps);
> > +
> > + err = rhashtable_insert_fast(&offdevs, &ondev->l, offdevs_params);
> > + if (err) {
> > + netdev_warn(netdev, "failed to register for BPF offload\n");
> > + goto err_unlock_free;
> > + }
> > +
> > + if (offdev)
> > + list_add(&ondev->offdev_netdevs, &offdev->netdevs);
> > + return 0;
> > +
> > +err_unlock_free:
> > + up_write(&bpf_devs_lock);
>
> No need to handle bpf_devs_lock in the "__" version of the register() helper?
> The goto label probably also needs another name, eg. "err_free".
Ah, not sure how I missed that, thanks!
> > + kfree(ondev);
> > + return err;
> > +}
> > +
>
> [ ... ]
>
> > +int bpf_prog_dev_bound_init(struct bpf_prog *prog, union bpf_attr *attr)
> > {
> > struct bpf_offload_netdev *ondev;
> > struct bpf_prog_offload *offload;
> > @@ -87,7 +198,7 @@ int bpf_prog_offload_init(struct bpf_prog *prog, union bpf_attr *attr)
> > attr->prog_type != BPF_PROG_TYPE_XDP)
> > return -EINVAL;
> >
> > - if (attr->prog_flags)
> > + if (attr->prog_flags & ~BPF_F_XDP_DEV_BOUND_ONLY)
> > return -EINVAL;
> >
> > offload = kzalloc(sizeof(*offload), GFP_USER);
> > @@ -102,11 +213,25 @@ int bpf_prog_offload_init(struct bpf_prog *prog, union bpf_attr *attr)
> > if (err)
> > goto err_maybe_put;
> >
> > + prog->aux->offload_requested = !(attr->prog_flags & BPF_F_XDP_DEV_BOUND_ONLY);
> > +
> > down_write(&bpf_devs_lock);
> > ondev = bpf_offload_find_netdev(offload->netdev);
> > if (!ondev) {
> > - err = -EINVAL;
> > - goto err_unlock;
> > + if (!bpf_prog_is_offloaded(prog->aux)) {
> > + /* When only binding to the device, explicitly
> > + * create an entry in the hashtable. See related
> > + * bpf_dev_bound_try_remove_netdev.
> > + */
> > + err = __bpf_offload_dev_netdev_register(NULL, offload->netdev);
> > + if (err)
> > + goto err_unlock;
> > + ondev = bpf_offload_find_netdev(offload->netdev);
> > + }
> > + if (!ondev) {
>
> nit. A bit confusing because the "ondev = bpf_offload_find_netdev(...)" above
> should not fail but "!ondev" is tested again here. I think the intention is to
> fail on the 'bpf_prog_is_offloaded() == true' case. May be:
>
> if (bpf_prog_is_offloaded(prog->aux)) {
> err = -EINVAL;
> goto err_unlock;
> }
> /* When only binding to the device, explicitly
> * ...
> */
> err = __bpf_offload_dev_netdev_register(NULL, offload->netdev);
> if (err)
> goto err_unlock;
> ondev = bpf_offload_find_netdev(offload->netdev);
>
Yeah, that looks better, thx!
> > + err = -EINVAL;
> > + goto err_unlock;
> > + }
> > }
> > offload->offdev = ondev->offdev;
> > prog->aux->offload = offload;
> > @@ -209,27 +334,28 @@ bpf_prog_offload_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt)
> > up_read(&bpf_devs_lock);
> > }
> >
> > -static void __bpf_prog_offload_destroy(struct bpf_prog *prog)
> > +static void bpf_dev_bound_try_remove_netdev(struct net_device *dev)
> > {
> > - struct bpf_prog_offload *offload = prog->aux->offload;
> > -
> > - if (offload->dev_state)
> > - offload->offdev->ops->destroy(prog);
> > + struct bpf_offload_netdev *ondev;
> >
> > - /* Make sure BPF_PROG_GET_NEXT_ID can't find this dead program */
> > - bpf_prog_free_id(prog, true);
> > + if (!dev)
> > + return;
> >
> > - list_del_init(&offload->offloads);
> > - kfree(offload);
> > - prog->aux->offload = NULL;
> > + ondev = bpf_offload_find_netdev(dev);
> > + if (ondev && !ondev->offdev && list_empty(&ondev->progs))
>
> hmm....list_empty(&ondev->progs) is tested here but will it be empty? ...
Ugh, yeah, need to move that list_del_init(&offload->offloads) to
somewhere before bpf_dev_bound_try_remove_netdev.
> > + __bpf_offload_dev_netdev_unregister(NULL, dev);
> > }
> >
> > -void bpf_prog_offload_destroy(struct bpf_prog *prog)
> > +void bpf_prog_dev_bound_destroy(struct bpf_prog *prog)
> > {
> > + rtnl_lock();
> > down_write(&bpf_devs_lock);
> > - if (prog->aux->offload)
> > - __bpf_prog_offload_destroy(prog);
> > + if (prog->aux->offload) {
> > + bpf_dev_bound_try_remove_netdev(prog->aux->offload->netdev);
>
> ... the "prog" here is still linked to ondev->progs, right?
> because __bpf_prog_dev_bound_destroy() is called later below.
Agreed, right.
> nit. May be the bpf_dev_bound_try_remove_netdev() should be folded/merged back
> into bpf_prog_dev_bound_destroy() to make things more clear.
Makes sense.
> > + __bpf_prog_dev_bound_destroy(prog); > + }
> > up_write(&bpf_devs_lock);
> > + rtnl_unlock();
> > }
>
> [ ... ]
>
> > +static int __init bpf_offload_init(void)
> > +{
> > + int err;
> > +
> > + down_write(&bpf_devs_lock);
>
> lock is probably not needed.
Sure, will drop.
> > + err = rhashtable_init(&offdevs, &offdevs_params);
> > + up_write(&bpf_devs_lock);
> > +
> > + return err;
> > +}
> > +
> > +late_initcall(bpf_offload_init);
>
> [ ... ]
>
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 5d51999cba30..194f8116aad4 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -9228,6 +9228,10 @@ static int dev_xdp_attach(struct net_device *dev, struct netlink_ext_ack *extack
> > NL_SET_ERR_MSG(extack, "Using offloaded program without HW_MODE flag is not supported");
> > return -EINVAL;
> > }
> > + if (bpf_prog_is_dev_bound(new_prog->aux) && !bpf_offload_dev_match(new_prog, dev)) {
> > + NL_SET_ERR_MSG(extack, "Program bound to different device");
> > + return -EINVAL;
> > + }
> > 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");
> > return -EINVAL;
> > @@ -10813,6 +10817,7 @@ void unregister_netdevice_many_notify(struct list_head *head,
> > /* Shutdown queueing discipline. */
> > dev_shutdown(dev);
> >
> > + bpf_dev_bound_netdev_unregister(dev);
>
> Does it matter if bpf_dev_bound_netdev_unregister(dev) is called before
> dev_xdp_uninstall(dev)? Asking because it seems more logic to unregister dev
> after detaching xdp progs.
By running it first I was hoping to catch any possible issues. Agreed
that doing it after makes more sense, will move.
> > dev_xdp_uninstall(dev);
> >
> > netdev_offload_xstats_disable_all(dev);
>
>
