Re: [PATCH bpf-next v5 08/17] bpf: Support consuming XDP HW metadata from fext programs

Martin KaFai Lau
2022-12-23T00:37:35Z

On 12/20/22 2:20 PM, Stanislav Fomichev wrote:
> diff --git a/kernel/bpf/offload.c b/kernel/bpf/offload.c
> index 0e3fc743e0a8..60978a1f9baa 100644
> --- a/kernel/bpf/offload.c
> +++ b/kernel/bpf/offload.c
> @@ -187,17 +187,13 @@ static void __bpf_offload_dev_netdev_unregister(struct bpf_offload_dev *offdev,
> kfree(ondev);
> }
>
> -int bpf_prog_dev_bound_init(struct bpf_prog *prog, union bpf_attr *attr)
> +static int __bpf_prog_dev_bound_init(struct bpf_prog *prog, struct net_device *netdev)
> {
> struct bpf_offload_netdev *ondev;
> struct bpf_prog_offload *offload;
> int err;
>
> - if (attr->prog_type != BPF_PROG_TYPE_SCHED_CLS &&
> - attr->prog_type != BPF_PROG_TYPE_XDP)
> - return -EINVAL;
> -
> - if (attr->prog_flags & ~BPF_F_XDP_DEV_BOUND_ONLY)
> + if (!netdev)

Is this !netdev test needed?

> return -EINVAL;
>
> offload = kzalloc(sizeof(*offload), GFP_USER);
> @@ -205,21 +201,13 @@ int bpf_prog_dev_bound_init(struct bpf_prog *prog, union bpf_attr *attr)
> return -ENOMEM;
>
> offload->prog = prog;
> + offload->netdev = netdev;
>
> - offload->netdev = dev_get_by_index(current->nsproxy->net_ns,
> - attr->prog_ifindex);
> - err = bpf_dev_offload_check(offload->netdev);
> - 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) {
> if (bpf_prog_is_offloaded(prog->aux)) {
> err = -EINVAL;
> - goto err_unlock;
> + goto err_free;
> }
>
> /* When only binding to the device, explicitly
> @@ -227,25 +215,80 @@ int bpf_prog_dev_bound_init(struct bpf_prog *prog, union bpf_attr *attr)
> */
> err = __bpf_offload_dev_netdev_register(NULL, offload->netdev);
> if (err)
> - goto err_unlock;
> + goto err_free;
> ondev = bpf_offload_find_netdev(offload->netdev);
> }
> offload->offdev = ondev->offdev;
> prog->aux->offload = offload;
> list_add_tail(&offload->offloads, &ondev->progs);
> - dev_put(offload->netdev);
> - up_write(&bpf_devs_lock);
>
> return 0;
> -err_unlock:
> - up_write(&bpf_devs_lock);
> -err_maybe_put:
> - if (offload->netdev)
> - dev_put(offload->netdev);
> +err_free:
> kfree(offload);
> return err;
> }
>
> +int bpf_prog_dev_bound_init(struct bpf_prog *prog, union bpf_attr *attr)
> +{
> + struct net_device *netdev;
> + int err;
> +
> + if (attr->prog_type != BPF_PROG_TYPE_SCHED_CLS &&
> + attr->prog_type != BPF_PROG_TYPE_XDP)
> + return -EINVAL;
> +
> + if (attr->prog_flags & ~BPF_F_XDP_DEV_BOUND_ONLY)
> + return -EINVAL;
> +
> + netdev = dev_get_by_index(current->nsproxy->net_ns, attr->prog_ifindex);
> + if (!netdev)
> + return -EINVAL;
> +
> + down_write(&bpf_devs_lock);
> + err = bpf_dev_offload_check(netdev);
> + if (err)
> + goto out;
> +
> + prog->aux->offload_requested = !(attr->prog_flags & BPF_F_XDP_DEV_BOUND_ONLY);

nit. move the bpf_dev_offload_check() and offload_requested assignment out. I
don't think they need lock protection so that it is clear what the lock is
protecting in the future reading. It seems the original code have them outside
also.

> +
> + err = __bpf_prog_dev_bound_init(prog, netdev);
> + if (err)
> + goto out;

nit. goto can be saved.

> +
> +out:
> + dev_put(netdev);
> + up_write(&bpf_devs_lock);
> + return err;
> +}
> +
> +int bpf_prog_dev_bound_inherit(struct bpf_prog *new_prog, struct bpf_prog *old_prog)
> +{
> + int err;
> +
> + if (!bpf_prog_is_dev_bound(old_prog->aux))
> + return 0;
> +
> + if (bpf_prog_is_offloaded(old_prog->aux))
> + return -EINVAL;
> +
> + down_write(&bpf_devs_lock);
> + if (!old_prog->aux->offload) {
> + err = -EINVAL;
> + goto out;
> + }
> +
> + new_prog->aux->dev_bound = old_prog->aux->dev_bound;
> + new_prog->aux->offload_requested = old_prog->aux->offload_requested;

nit. Same here, I think the initialization can be moved outside of the lock.

> +
> + err = __bpf_prog_dev_bound_init(new_prog, old_prog->aux->offload->netdev);
> + if (err)
> + goto out;

goto can be saved.

> +
> +out:
> + up_write(&bpf_devs_lock);
> + return err;
> +}
> +
> int bpf_prog_offload_verifier_prep(struct bpf_prog *prog)
> {
> struct bpf_prog_offload *offload;
> @@ -687,6 +730,22 @@ bool bpf_offload_dev_match(struct bpf_prog *prog, struct net_device *netdev)
> }
> EXPORT_SYMBOL_GPL(bpf_offload_dev_match);
>
> +bool bpf_prog_dev_bound_match(struct bpf_prog *lhs, struct bpf_prog *rhs)
> +{
> + bool ret;
> +
> + if (bpf_prog_is_offloaded(lhs->aux) != bpf_prog_is_offloaded(rhs->aux))
> + return false;
> +
> + down_read(&bpf_devs_lock);
> + ret = lhs->aux->offload && rhs->aux->offload &&
> + lhs->aux->offload->netdev &&
> + lhs->aux->offload->netdev == rhs->aux->offload->netdev;
> + up_read(&bpf_devs_lock);
> +
> + return ret;
> +}
> +
> bool bpf_offload_prog_map_match(struct bpf_prog *prog, struct bpf_map *map)
> {
> struct bpf_offloaded_map *offmap;
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 11c558be4992..64a68e8fb072 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -2605,6 +2605,12 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr)
> goto free_prog_sec;
> }
>
> + if (type == BPF_PROG_TYPE_EXT && dst_prog) {

Does it also need to test the bpf_prog_is_dev_bound(dst_prog->aux)? Otherwise,
the bpf_prog_dev_bound_inherit() below will fail on everything for !CONFIG_NET.

> + err = bpf_prog_dev_bound_inherit(prog, dst_prog);
> + if (err)
> + goto free_prog_sec;
> + }
> +
> /* find program type: socket_filter vs tracing_filter */
> err = find_prog_type(type, prog);
> if (err < 0)
> @@ -3021,6 +3027,12 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
> goto out_put_prog;
> }
>
> + if (bpf_prog_is_dev_bound(prog->aux) &&

Like here.

> + !bpf_prog_dev_bound_match(prog, tgt_prog)) {
> + err = -EINVAL;
> + goto out_put_prog;
> + }
> +
> key = bpf_trampoline_compute_key(tgt_prog, NULL, btf_id);
> }
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 320451a0be3e..64f4d2b5824f 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -16537,11 +16537,6 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
> if (tgt_prog) {
> struct bpf_prog_aux *aux = tgt_prog->aux;
>
> - if (bpf_prog_is_dev_bound(tgt_prog->aux)) {
> - bpf_log(log, "Replacing device-bound programs not supported\n");
> - return -EINVAL;
> - }
> -
> for (i = 0; i < aux->func_info_cnt; i++)
> if (aux->func_info[i].type_id == btf_id) {
> subprog = i;