On Wed, Dec 13, 2023 at 9:43 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx>
> > ---
>
> Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
>
> [...]
>
> > @@ -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.
I think it's actually more convoluted and error-prone with the is_ctx
flag. Tag is overriding whatever BTF type information we have. But if
we delay ARG_PTR_TO_CTX setting to later, we need to make sure that
post-tag BTF processing is aware of is_ctx (and potentially other)
flags and doesn't freak out. We might add more BTF processing before
ARG_PTR_TO_CTX, etc. Having to worry about not overriding tag-based
decisions seems very error-prone.
Also, btf_prepare_func_args() really simplifies the "definition" of an
argument to one enum (and in some cases maybe an extra argument like
memory size). It should be fine to just set this enum in two places
separately, doesn't seem like a hindrance for maintainability.
>
> [...]
>
>
