Re: [PATCH bpf-next v9 2/4] bpf: Allow trusted pointers to be passed to KF_TRUSTED_ARGS kfuncs

[David Vernet]

On Sun, Nov 20, 2022 at 09:28:15AM -0800, Alexei Starovoitov wrote:
> On Sat, Nov 19, 2022 at 11:10:02PM -0600, David Vernet wrote:
> >  		case KF_ARG_PTR_TO_BTF_ID:
> >  			/* Only base_type is checked, further checks are done here */
> > -			if (reg->type != PTR_TO_BTF_ID &&
> > -			    (!reg2btf_ids[base_type(reg->type)] || type_flag(reg->type))) {
> > -				verbose(env, "arg#%d expected pointer to btf or socket\n", i);
> > +			if ((base_type(reg->type) != PTR_TO_BTF_ID ||
> > +			     bpf_type_has_unsafe_modifiers(reg->type)) &&
> > +			    !reg2btf_ids[base_type(reg->type)]) {
> > +				verbose(env, "arg#%d is %s ", i, reg_type_str(env, reg->type));
> > +				verbose(env, "expected %s or socket\n",
> > +					reg_type_str(env, base_type(reg->type) |
> > +							  (type_flag(reg->type) & BPF_REG_TRUSTED_MODIFIERS)));
> ...
> > diff --git a/tools/testing/selftests/bpf/verifier/calls.c b/tools/testing/selftests/bpf/verifier/calls.c
> > index 86d6fef2e3b4..3193915c5ee6 100644
> > --- a/tools/testing/selftests/bpf/verifier/calls.c
> > +++ b/tools/testing/selftests/bpf/verifier/calls.c
> > @@ -109,7 +109,7 @@
> >  	},
> >  	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
> >  	.result = REJECT,
> > -	.errstr = "arg#0 expected pointer to btf or socket",
> > +	.errstr = "arg#0 is ptr_or_null_ expected ptr_ or socket",
> 
> Nice.
> I missed the fact that reg_type_str() prints only the type.
> We see more verbose prints in print_verifier_state():
>   verbose(env, "%s", reg_type_str(env, t));
>   if (base_type(t) == PTR_TO_BTF_ID)
>           verbose(env, "%s", kernel_type_name(reg->btf, reg->btf_id));
> Since reg_type_str() prints into a buffer maybe we can enhance it with
> struct name printing too?

I like that, and we have a bit of extra space after bumping
TYPE_STR_BUF_LEN to 128 so why not. I'll take care of it in a follow-up
change.

> Not urgent.
> The set looks great. Applied.

Thanks!

> There is an odd arm64 failure in bonding test reported by CI, but looks unrelated.

Hmmm yeah, can't see how this change would have affected that. I'll keep
an eye on it in CI to see if it persists.