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

[Martin KaFai Lau <martin.lau@xxxxxxxxx>]

On 1/4/23 1:59 PM, Stanislav Fomichev wrote:
> -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)

nit. I think this check is also unnecessary.

[ ... ]

> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 191a4312f4b7..2ec2f53eeff6 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -2605,6 +2605,13 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr)
>   			goto free_prog_sec;
>   	}
>   
> +	if (type == BPF_PROG_TYPE_EXT && dst_prog &&
> +	    bpf_prog_is_dev_bound(dst_prog->aux)) {
> +		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 +3028,12 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
>   			goto out_put_prog;
>   		}
>   
> +		if (bpf_prog_is_dev_bound(prog->aux) &&
> +		    !bpf_prog_dev_bound_match(prog, tgt_prog)) {
> +			err = -EINVAL;
> +			goto out_put_prog;
> +		}

This looks good.  One minor comment...

> +
>   		key = bpf_trampoline_compute_key(tgt_prog, NULL, btf_id);
>   	}
>   
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 0d0a49a2c5fd..8c1b1259f30b 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -16531,11 +16531,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;
> -		}

... can the above "bpf_prog_is_dev_bound(prog->aux) &&..." check in syscall.c be 
done in the bpf_check_attach_target() here?  Mentally that seems to belong more 
to bpf_check_attach_target().

> -
>   		for (i = 0; i < aux->func_info_cnt; i++)
>   			if (aux->func_info[i].type_id == btf_id) {
>   				subprog = i;