Re: [PATCH bpf-next v10 11/24] bpf: Rewrite kfunc argument handling

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

On Sat, Nov 19, 2022 at 01:10:12AM IST, David Vernet wrote:
> On Fri, Nov 18, 2022 at 07:26:01AM +0530, Kumar Kartikeya Dwivedi wrote:
> > As we continue to add more features, argument types, kfunc flags, and
> > different extensions to kfuncs, the code to verify the correctness of
> > the kfunc prototype wrt the passed in registers has become ad-hoc and
> > ugly to read. To make life easier, and make a very clear split between
> > different stages of argument processing, move all the code into
> > verifier.c and refactor into easier to read helpers and functions.
> >
> > This also makes sharing code within the verifier easier with kfunc
> > argument processing. This will be more and more useful in later patches
> > as we are now moving to implement very core BPF helpers as kfuncs, to
> > keep them experimental before baking into UAPI.
> >
> > Remove all kfunc related bits now from btf_check_func_arg_match, as
> > users have been converted away to refactored kfunc argument handling.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
>
> Thanks for working on this. I'm relieved to see this work being done. I
> have a few comments but overall this is great. I'll take a closer look
> later.
>
> > ---
> > [...]
> > +static bool is_kfunc_arg_mem_size(const struct btf *btf,
> > +				  const struct btf_param *arg,
> > +				  const struct bpf_reg_state *reg)
> > +{
> > +	int len, sfx_len = sizeof("__sz") - 1;
> > +	const struct btf_type *t;
> > +	const char *param_name;
> > +
> > +	t = btf_type_skip_modifiers(btf, arg->type, NULL);
> > +	if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE)
> > +		return false;
> > +
> > +	/* In the future, this can be ported to use BTF tagging */
> > +	param_name = btf_name_by_offset(btf, arg->name_off);
> > +	if (str_is_empty(param_name))
> > +		return false;
> > +	len = strlen(param_name);
> > +	if (len < sfx_len)
> > +		return false;
> > +	param_name += len - sfx_len;
> > +	if (strncmp(param_name, "__sz", sfx_len))
> > +		return false;
>
> Oh, I thought we weren't doing this arg-type-by-name-suffix thing? I see
> that you're just moving it around so it's fine to move it here, but it
> seems to diverge from the discussions I've seen in the past. Don't want
> to distract from the patch but is there a plan to get rid of this at
> some point?
>

I think unless we have BTF tags in GCC, it's not possible to drop this suffix
based tagging. Also not sure I remember/was part of the discussion you're
referring to.

> > +
> > +	return true;
> > +}
> > +
> > +static bool is_kfunc_arg_scalar_with_name(const struct btf *btf,
> > +					  const struct btf_param *arg,
> > +					  const char *name)
> > +{
> > +	int len, target_len = strlen(name);
> > +	const char *param_name;
> > +
> > +	param_name = btf_name_by_offset(btf, arg->name_off);
> > +	if (str_is_empty(param_name))
> > +		return false;
> > +	len = strlen(param_name);
> > +	if (len != target_len)
> > +		return false;
> > +	if (strcmp(param_name, name))
> > +		return false;
> > +
> > +	return true;
> > +}
>
> It doesn't look like this function has anything to do with the arg being
> a scalar, does it? Should we just rename it something like,
> "kfunc_arg_has_name()"?
>

The scalar_with_name was a suggestion from Alexei, but I think it's fine.
Since this already got applied not sure it's worth it now.

> > [...]
> > +static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_arg_meta *meta)
> > +{
> > +	const char *func_name = meta->func_name, *ref_tname;
> > +	const struct btf *btf = meta->btf;
> > +	const struct btf_param *args;
> > +	u32 i, nargs;
> > +	int ret;
> > +
> > +	args = (const struct btf_param *)(meta->func_proto + 1);
>
> Not your change and it's fine to not block this cleanup on fixing an
> issue that's been in the verifier for a while, but it's unfortunate that
> we never built proper encapsulations for fetching params from the func
> proto. This is pretty brittle. A cleanup for another day...
>

This was just kept same as the older code, but we do have an accessor for this
case: btf_params (in include/linux/btf.h). I will roll it in with a few other
clean ups (or you can beat me to it).

> [...]
> > -	if (acq && !btf_type_is_struct_ptr(desc_btf, t)) {
> > +	if (is_kfunc_acquire(&meta) && !btf_type_is_struct_ptr(meta.btf, t)) {
> >  		verbose(env, "acquire kernel function does not return PTR_TO_BTF_ID\n");
> >  		return -EINVAL;
> >  	}
>
> Can you move this up to where we check is_kfunc_desctructive(),
> is_kfunc_sleepable(), etc to group logically similar code together?
>

I think I prefer it here, unlike those checks which apply to the kfunc, this is
located along with other checks for the return type, post argument processing.
But let me know if you think otherwise.