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

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

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;
+
        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;
        }

?