The offload tests still pass after this, right?
TBH I don't remember this code well enough to spot major issues.
On Mon, 5 Dec 2022 18:45:45 -0800 Stanislav Fomichev wrote:
> There is an ndo handler per kfunc, the verifier replaces a call to the
> generic kfunc with a call to the per-device one.
>
> For XDP, we define a new kfunc set (xdp_metadata_kfunc_ids) which
> implements all possible metatada kfuncs. Not all devices have to
> implement them. If kfunc is not supported by the target device,
> the default implementation is called instead.
>
> Upon loading, if BPF_F_XDP_HAS_METADATA is passed via prog_flags,
> we treat prog_index as target device for kfunc resolution.
> @@ -2476,10 +2477,18 @@ void bpf_offload_dev_netdev_unregister(struct bpf_offload_dev *offdev,
> struct net_device *netdev);
> bool bpf_offload_dev_match(struct bpf_prog *prog, struct net_device *netdev);
>
> +void *bpf_offload_resolve_kfunc(struct bpf_prog *prog, u32 func_id);
There seems to be some mis-naming going on. I expected:
offloaded =~ nfp
dev_bound == XDP w/ funcs
*_offload_resolve_kfunc looks misnamed? Unless you want to resolve
for HW offload?
> void unpriv_ebpf_notify(int new_state);
>
> #if defined(CONFIG_NET) && defined(CONFIG_BPF_SYSCALL)
> int bpf_prog_offload_init(struct bpf_prog *prog, union bpf_attr *attr);
> +void bpf_offload_bound_netdev_unregister(struct net_device *dev);
ditto: offload_bound is a mix of terms no?
> @@ -1611,6 +1612,10 @@ struct net_device_ops {
> ktime_t (*ndo_get_tstamp)(struct net_device *dev,
> const struct skb_shared_hwtstamps *hwtstamps,
> bool cycles);
> + bool (*ndo_xdp_rx_timestamp_supported)(const struct xdp_md *ctx);
> + u64 (*ndo_xdp_rx_timestamp)(const struct xdp_md *ctx);
> + bool (*ndo_xdp_rx_hash_supported)(const struct xdp_md *ctx);
> + u32 (*ndo_xdp_rx_hash)(const struct xdp_md *ctx);
> };
Is this on the fast path? Can we do an indirection?
Put these ops in their own struct and add a pointer to that struct
in net_device_ops? Purely for grouping reasons because the netdev
ops are getting orders of magnitude past the size where you can
actually find stuff in this struct.
> bpf_free_used_maps(aux);
> bpf_free_used_btfs(aux);
> - if (bpf_prog_is_offloaded(aux))
> + if (bpf_prog_is_dev_bound(aux))
> bpf_prog_offload_destroy(aux->prog);
This also looks a touch like a mix of terms (condition vs function
called).
> +static int __bpf_offload_init(void);
> +static int __bpf_offload_dev_netdev_register(struct bpf_offload_dev *offdev,
> + struct net_device *netdev);
> +static void __bpf_offload_dev_netdev_unregister(struct bpf_offload_dev *offdev,
> + struct net_device *netdev);
fwd declarations are yuck
> static int bpf_dev_offload_check(struct net_device *netdev)
> {
> if (!netdev)
> @@ -87,13 +93,17 @@ 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_HAS_METADATA)
> return -EINVAL;
>
> offload = kzalloc(sizeof(*offload), GFP_USER);
> if (!offload)
> return -ENOMEM;
>
> + err = __bpf_offload_init();
> + if (err)
> + return err;
leaks offload
> @@ -209,6 +233,19 @@ bpf_prog_offload_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt)
> up_read(&bpf_devs_lock);
> }
>
> +static void maybe_remove_bound_netdev(struct net_device *dev)
> +{
func name prefix ?
> -struct bpf_offload_dev *
> -bpf_offload_dev_create(const struct bpf_prog_offload_ops *ops, void *priv)
> +static int __bpf_offload_init(void)
> {
> - struct bpf_offload_dev *offdev;
> int err;
>
> down_write(&bpf_devs_lock);
> @@ -680,12 +740,25 @@ bpf_offload_dev_create(const struct bpf_prog_offload_ops *ops, void *priv)
> err = rhashtable_init(&offdevs, &offdevs_params);
> if (err) {
> up_write(&bpf_devs_lock);
> - return ERR_PTR(err);
> + return err;
> }
> offdevs_inited = true;
> }
> up_write(&bpf_devs_lock);
>
> + return 0;
> +}
Would late_initcall() or some such not work for this?
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 5b221568dfd4..862e03fcffa6 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 device-bound program without HW_MODE flag is not supported");
extack should get updated here, I reckon, maybe in previous patch
> return -EINVAL;
> }
> + if (bpf_prog_is_dev_bound(new_prog->aux) && !bpf_offload_dev_match(new_prog, dev)) {
bound_dev_match() ?
> + NL_SET_ERR_MSG(extack, "Cannot attach to a different target device");
different than.. ?
> + 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;
