Re: [PATCH bpf-next v4 06/15] bpf: Support consuming XDP HW metadata from fext programs

[Martin KaFai Lau <martin.lau@linux.dev>]

On 12/12/22 6:35 PM, Stanislav Fomichev wrote:
> From: Toke Høiland-Jørgensen <toke@redhat.com>
> 
> Instead of rejecting the attaching of PROG_TYPE_EXT programs to XDP
> programs that consume HW metadata, implement support for propagating the
> offload information. The extension program doesn't need to set a flag or
> ifindex, it these will just be propagated from the target by the verifier.

s/it/because/ ... these will just be propagated....

> We need to create a separate offload object for the extension program,
> though, since it can be reattached to a different program later (which
> means we can't just inhering the offload information from the target).

hmm.... inheriting?

> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 11c558be4992..8686475f0dbe 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -3021,6 +3021,14 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
>   			goto out_put_prog;
>   		}
>   
> +		if (bpf_prog_is_dev_bound(prog->aux) &&


> +		    (bpf_prog_is_offloaded(tgt_prog->aux) ||
> +		     !bpf_prog_is_dev_bound(tgt_prog->aux) ||
> +		     !bpf_offload_dev_match(prog, tgt_prog->aux->offload->netdev))) {

hmm... tgt_prog->aux->offload does not look safe without taking bpf_devs_lock. 
offload could be NULL, no?

It probably needs a bpf_prog_dev_bound_match(prog, tgt_prog) which takes the lock.

> +			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 e61fe0472b9b..5c6d6d61e57a 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -16524,11 +16524,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;
> @@ -16789,10 +16784,22 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
>   	if (tgt_prog && prog->type == BPF_PROG_TYPE_EXT) {
>   		/* to make freplace equivalent to their targets, they need to
>   		 * inherit env->ops and expected_attach_type for the rest of the
> -		 * verification
> +		 * verification; we also need to propagate the prog offload data
> +		 * for resolving kfuncs.
>   		 */
>   		env->ops = bpf_verifier_ops[tgt_prog->type];
>   		prog->expected_attach_type = tgt_prog->expected_attach_type;
> +
> +		if (bpf_prog_is_dev_bound(tgt_prog->aux)) {
> +			if (bpf_prog_is_offloaded(tgt_prog->aux))
> +				return -EINVAL;
> +
> +			prog->aux->dev_bound = true;
> +			ret = __bpf_prog_dev_bound_init(prog,
> +							tgt_prog->aux->offload->netdev);

Same here for tgt_prog->aux->offload.  bpf_prog_dev_bound_init() will need to 
take an extra dst_prog arg, like bpf_prog_dev_bound_init(prog, attr, dst_prog). 
It should be called earlier in syscall.c.

> +			if (ret)
> +				return ret;
> +		}
>   	}
>   
>   	/* store info about the attachment target that will be used later */