Re: [PATCH v2 bpf-next 06/10] bpf: support 'arg:xxx' btf_decl_tag-based hints for global subprog args

[Eduard Zingerman <eddyz87@gmail.com>]

On Tue, 2023-12-12 at 15:25 -0800, Andrii Nakryiko wrote:
> Add support for annotating global BPF subprog arguments to provide more
> information about expected semantics of the argument. Currently,
> verifier relies purely on argument's BTF type information, and supports
> three general use cases: scalar, pointer-to-context, and
> pointer-to-fixed-size-memory.
> 
> Scalar and pointer-to-fixed-mem work well in practice and are quite
> natural to use. But pointer-to-context is a bit problematic, as typical
> BPF users don't realize that they need to use a special type name to
> signal to verifier that argument is not just some pointer, but actually
> a PTR_TO_CTX. Further, even if users do know which type to use, it is
> limiting in situations where the same BPF program logic is used across
> few different program types. Common case is kprobes, tracepoints, and
> perf_event programs having a helper to send some data over BPF perf
> buffer. bpf_perf_event_output() requires `ctx` argument, and so it's
> quite cumbersome to share such global subprog across few BPF programs of
> different types, necessitating extra static subprog that is context
> type-agnostic.
> 
> Long story short, there is a need to go beyond types and allow users to
> add hints to global subprog arguments to define expectations.
> 
> This patch adds such support for two initial special tags:
>   - pointer to context;
>   - non-null qualifier for generic pointer arguments.
> 
> All of the above came up in practice already and seem generally useful
> additions. Non-null qualifier is an often requested feature, which
> currently has to be worked around by having unnecessary NULL checks
> inside subprogs even if we know that arguments are never NULL. Pointer
> to context was discussed earlier.
> 
> As for implementation, we utilize btf_decl_tag attribute and set up an
> "arg:xxx" convention to specify argument hint. As such:
>   - btf_decl_tag("arg:ctx") is a PTR_TO_CTX hint;
>   - btf_decl_tag("arg:nonnull") marks pointer argument as not allowed to
>     be NULL, making NULL check inside global subprog unnecessary.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

> @@ -6846,7 +6846,35 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
>  	 * Only PTR_TO_CTX and SCALAR are supported atm.
>  	 */
>  	for (i = 0; i < nargs; i++) {
> +		bool is_nonnull = false;
> +		const char *tag;
> +
>  		t = btf_type_by_id(btf, args[i].type);
> +
> +		tag = btf_find_decl_tag_value(btf, fn_t, i, "arg:");
> +		if (IS_ERR(tag) && PTR_ERR(tag) == -ENOENT) {
> +			tag = NULL;
> +		} else if (IS_ERR(tag)) {
> +			bpf_log(log, "arg#%d type's tag fetching failure: %ld\n", i, PTR_ERR(tag));
> +			return PTR_ERR(tag);
> +		}
> +		/* 'arg:<tag>' decl_tag takes precedence over derivation of
> +		 * register type from BTF type itself
> +		 */
> +		if (tag) {
> +			/* disallow arg tags in static subprogs */
> +			if (!is_global) {
> +				bpf_log(log, "arg#%d type tag is not supported in static functions\n", i);
> +				return -EOPNOTSUPP;
> +			}
> +			if (strcmp(tag, "ctx") == 0) {
> +				sub->args[i].arg_type = ARG_PTR_TO_CTX;
> +				continue;

Nit: personally, I'd keep tags parsing and processing logically separate:
     - at this point set a flag 'is_ctx'
     - and modify the check below as follows:
     
		if (is_ctx || (btf_type_is_ptr(t) && btf_get_prog_ctx_type(log, btf, t, prog_type, i))) {
			sub->args[i].arg_type = ARG_PTR_TO_CTX;
			continue;
		}
    
     So that there is only one place where ARG_PTR_TO_CTX is assigned.
     Feel free to ignore.

[...]