Re: [PATCH bpf-next 02/16] bpf: Recognize '__map' suffix in kfunc arguments

[David Vernet <void@manifault.com>]

[-- Attachment #1: Type: text/plain, Size: 7051 bytes --]

On Fri, Feb 09, 2024 at 09:46:57AM -0800, Alexei Starovoitov wrote:
> On Fri, Feb 09, 2024 at 10:57:45AM -0600, David Vernet wrote:
> > On Tue, Feb 06, 2024 at 02:04:27PM -0800, Alexei Starovoitov wrote:
> > > From: Alexei Starovoitov <ast@kernel.org>
> > > 
> > > Recognize 'void *p__map' kfunc argument as 'struct bpf_map *p__map'.
> > > It allows kfunc to have 'void *' argument for maps, since bpf progs
> > > will call them as:
> > > struct {
> > >         __uint(type, BPF_MAP_TYPE_ARENA);
> > > 	...
> > > } arena SEC(".maps");
> > > 
> > > bpf_kfunc_with_map(... &arena ...);
> > > 
> > > Underneath libbpf will load CONST_PTR_TO_MAP into the register via ld_imm64 insn.
> > > If kfunc was defined with 'struct bpf_map *' it would pass
> > > the verifier, but bpf prog would need to use '(void *)&arena'.
> > > Which is not clean.
> > > 
> > > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> > > ---
> > >  kernel/bpf/verifier.c | 14 +++++++++++++-
> > >  1 file changed, 13 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index d9c2dbb3939f..db569ce89fb1 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -10741,6 +10741,11 @@ static bool is_kfunc_arg_ignore(const struct btf *btf, const struct btf_param *a
> > >  	return __kfunc_param_match_suffix(btf, arg, "__ign");
> > >  }
> > >  
> > > +static bool is_kfunc_arg_map(const struct btf *btf, const struct btf_param *arg)
> > > +{
> > > +	return __kfunc_param_match_suffix(btf, arg, "__map");
> > > +}
> > > +
> > >  static bool is_kfunc_arg_alloc_obj(const struct btf *btf, const struct btf_param *arg)
> > >  {
> > >  	return __kfunc_param_match_suffix(btf, arg, "__alloc");
> > > @@ -11064,7 +11069,7 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_env *env,
> > >  		return KF_ARG_PTR_TO_CONST_STR;
> > >  
> > >  	if ((base_type(reg->type) == PTR_TO_BTF_ID || reg2btf_ids[base_type(reg->type)])) {
> > > -		if (!btf_type_is_struct(ref_t)) {
> > > +		if (!btf_type_is_struct(ref_t) && !btf_type_is_void(ref_t)) {
> > >  			verbose(env, "kernel function %s args#%d pointer type %s %s is not supported\n",
> > >  				meta->func_name, argno, btf_type_str(ref_t), ref_tname);
> > >  			return -EINVAL;
> > > @@ -11660,6 +11665,13 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
> > >  		if (kf_arg_type < 0)
> > >  			return kf_arg_type;
> > >  
> > > +		if (is_kfunc_arg_map(btf, &args[i])) {
> > > +			/* If argument has '__map' suffix expect 'struct bpf_map *' */
> > > +			ref_id = *reg2btf_ids[CONST_PTR_TO_MAP];
> > > +			ref_t = btf_type_by_id(btf_vmlinux, ref_id);
> > > +			ref_tname = btf_name_by_offset(btf, ref_t->name_off);
> > > +		}
> > 
> > This is fine, but given that this should only apply to KF_ARG_PTR_TO_BTF_ID,
> > this seems a bit cleaner, wdyt?
> > 
> > index ddaf09db1175..998da8b302ac 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -10741,6 +10741,11 @@ static bool is_kfunc_arg_ignore(const struct btf *btf, const struct btf_param *a
> >         return __kfunc_param_match_suffix(btf, arg, "__ign");
> >  }
> > 
> > +static bool is_kfunc_arg_map(const struct btf *btf, const struct btf_param *arg)
> > +{
> > +       return __kfunc_param_match_suffix(btf, arg, "__map");
> > +}
> > +
> >  static bool is_kfunc_arg_alloc_obj(const struct btf *btf, const struct btf_param *arg)
> >  {
> >         return __kfunc_param_match_suffix(btf, arg, "__alloc");
> > @@ -10910,6 +10915,7 @@ enum kfunc_ptr_arg_type {
> >         KF_ARG_PTR_TO_RB_NODE,
> >         KF_ARG_PTR_TO_NULL,
> >         KF_ARG_PTR_TO_CONST_STR,
> > +       KF_ARG_PTR_TO_MAP,      /* pointer to a struct bpf_map */
> >  };
> > 
> >  enum special_kfunc_type {
> > @@ -11064,12 +11070,12 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_env *env,
> >                 return KF_ARG_PTR_TO_CONST_STR;
> > 
> >         if ((base_type(reg->type) == PTR_TO_BTF_ID || reg2btf_ids[base_type(reg->type)])) {
> > -               if (!btf_type_is_struct(ref_t)) {
> > +               if (!btf_type_is_struct(ref_t) && !btf_type_is_void(ref_t)) {
> >                         verbose(env, "kernel function %s args#%d pointer type %s %s is not supported\n",
> >                                 meta->func_name, argno, btf_type_str(ref_t), ref_tname);
> >                         return -EINVAL;
> >                 }
> > -               return KF_ARG_PTR_TO_BTF_ID;
> > +               return is_kfunc_arg_map(meta->btf, &args[argno]) ? KF_ARG_PTR_TO_MAP : KF_ARG_PTR_TO_BTF_ID;
> 
> Makes sense, but then should I add the following on top:
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index e970d9fd7f32..b524dc168023 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -11088,13 +11088,16 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_env *env,
>         if (is_kfunc_arg_const_str(meta->btf, &args[argno]))
>                 return KF_ARG_PTR_TO_CONST_STR;
> 
> +       if (is_kfunc_arg_map(meta->btf, &args[argno]))
> +               return KF_ARG_PTR_TO_MAP;
> +

Yeah, it's probably cleaner to pull it out of that block, which is
already a bit of a mess.

Only thing is that it doesn't make sense to invoke is_kfunc_arg_map() on
something that doesn't have base_type(reg->type) == CONST_PTR_TO_MAP
right? We sort of had that covered in the below block beacuse of the
reg2btf_ids[base_type(reg->type)] check, but even then it was kind of
sketchy because we could have base_type(reg->type) == PTR_TO_BTF_ID or
some other base_type with a nonzero btf ID and still treat it as a
KF_ARG_PTR_TO_MAP depending on how the kfunc was named. So maybe
something like this would be yet another improvement on top of both
proposals that would avoid any weird edge cases or confusion on the part
of the kfunc author?

+ if (is_kfunc_arg_map(meta->btf, &args[argno])) {
+         if (base_type(reg->type) != CONST_PTR_TO_MAP) {
+                 verbose(env, "kernel function %s map arg#%d %s reg was not type %s\n",
+                         meta->func_name, argno, ref_name, reg_type_str(env, CONST_PTR_TO_MAP));
+                 return -EINVAL;
+         }
+         return KF_ARG_PTR_TO_MAP;
+ }
+

>         if ((base_type(reg->type) == PTR_TO_BTF_ID || reg2btf_ids[base_type(reg->type)])) {
> -               if (!btf_type_is_struct(ref_t) && !btf_type_is_void(ref_t)) {
> +               if (!btf_type_is_struct(ref_t)) {
>                         verbose(env, "kernel function %s args#%d pointer type %s %s is not supported\n",
>                                 meta->func_name, argno, btf_type_str(ref_t), ref_tname);
>                         return -EINVAL;
>                 }
> -               return is_kfunc_arg_map(meta->btf, &args[argno]) ? KF_ARG_PTR_TO_MAP : KF_ARG_PTR_TO_BTF_ID;
> +               return KF_ARG_PTR_TO_BTF_ID;
>         }
> 
> ?
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]