On Sat, Mar 05, 2022 at 01:45:39AM IST, Martin KaFai Lau wrote:
> On Fri, Mar 04, 2022 at 05:35:01AM +0530, Kumar Kartikeya Dwivedi wrote:
> > Lift the list of register types allowed for having fixed and variable
> > offsets when passed as helper function arguments into a common helper,
> > so that they can be reused for kfunc checks in later commits. Keeping a
> > common helper aids maintainability and allows us to follow the same
> > consistent rules across helpers and kfuncs. Also, convert check_func_arg
> > to use this function.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> > ---
> > include/linux/bpf_verifier.h | 3 ++
> > kernel/bpf/verifier.c | 69 +++++++++++++++++++++---------------
> > 2 files changed, 44 insertions(+), 28 deletions(-)
> >
> > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> > index 7a7be8c057f2..38b24ee8d8c2 100644
> > --- a/include/linux/bpf_verifier.h
> > +++ b/include/linux/bpf_verifier.h
> > @@ -521,6 +521,9 @@ bpf_prog_offload_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt);
> >
> > int check_ptr_off_reg(struct bpf_verifier_env *env,
> > const struct bpf_reg_state *reg, int regno);
> > +int check_func_arg_reg_off(struct bpf_verifier_env *env,
> > + const struct bpf_reg_state *reg, int regno,
> > + enum bpf_arg_type arg_type);
> > int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> > u32 regno);
> > int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index a57db4b2803c..c85f4b2458f4 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -5359,6 +5359,44 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
> > return 0;
> > }
> >
> > +int check_func_arg_reg_off(struct bpf_verifier_env *env,
> > + const struct bpf_reg_state *reg, int regno,
> > + enum bpf_arg_type arg_type)
> > +{
> > + enum bpf_reg_type type = reg->type;
> > + int err;
> > +
> > + switch ((u32)type) {
> > + case SCALAR_VALUE:
> > + /* Pointer types where reg offset is explicitly allowed: */
> > + case PTR_TO_PACKET:
> > + case PTR_TO_PACKET_META:
> > + case PTR_TO_MAP_KEY:
> > + case PTR_TO_MAP_VALUE:
> > + case PTR_TO_MEM:
> > + case PTR_TO_MEM | MEM_RDONLY:
> > + case PTR_TO_MEM | MEM_ALLOC:
> > + case PTR_TO_BUF:
> > + case PTR_TO_BUF | MEM_RDONLY:
> > + case PTR_TO_STACK:
> > + /* Some of the argument types nevertheless require a
> > + * zero register offset.
> > + */
> > + if (arg_type == ARG_PTR_TO_ALLOC_MEM)
> > + goto force_off_check;
> > + break;
> > + /* All the rest must be rejected: */
> > + default:
> > +force_off_check:
> > + err = __check_ptr_off_reg(env, reg, regno,
> > + type == PTR_TO_BTF_ID);
> > + if (err < 0)
> > + return err;
> Nit. Since it is refactoring to a new function now,
> it can directly return __check_ptr_off_reg().
>
>
> > + break;
> > + }
> > + return 0;
> > +}
> May as well flip the arg_type check and leave calling
> the __check_ptr_off_reg at the end.
>
> Uncompiled code:
>
> int check_func_arg_reg_off(struct bpf_verifier_env *env,
> const struct bpf_reg_state *reg, int regno,
> enum bpf_arg_type arg_type)
> {
> enum bpf_reg_type type = reg->type;
> bool fixed_off_ok = false;
>
> switch ((u32)type) {
> case SCALAR_VALUE:
> /* Pointer types where reg offset is explicitly allowed: */
> case PTR_TO_PACKET:
> case PTR_TO_PACKET_META:
> case PTR_TO_MAP_KEY:
> case PTR_TO_MAP_VALUE:
> case PTR_TO_MEM:
> case PTR_TO_MEM | MEM_RDONLY:
> case PTR_TO_MEM | MEM_ALLOC:
> case PTR_TO_BUF:
> case PTR_TO_BUF | MEM_RDONLY:
> case PTR_TO_STACK:
> /* Some of the argument types nevertheless require a
> * zero register offset.
> */
> if (arg_type != ARG_PTR_TO_ALLOC_MEM)
> return 0;
> break;
> case PTR_TO_BTF_ID:
> fixed_off_ok = true;
> break;
> }
>
> return __check_ptr_off_reg(env, reg, regno, fixed_off_ok);
> }
>
> Then 'case PTR_TO_BTF_ID' can then be reused in patch 4 instead
> of adding a special 'if (type == PTR_TO_BTF_ID)' just
> before the 'switch ((u32)type)'
>
> Both are nits. can be ignored.
>
Both suggestions are good, will wait for discussion to conclude and respin.
> > +
> > static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> > struct bpf_call_arg_meta *meta,
> > const struct bpf_func_proto *fn)
> > @@ -5408,34 +5446,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> > if (err)
> > return err;
> >
> > - switch ((u32)type) {
> > - case SCALAR_VALUE:
> > - /* Pointer types where reg offset is explicitly allowed: */
> > - case PTR_TO_PACKET:
> > - case PTR_TO_PACKET_META:
> > - case PTR_TO_MAP_KEY:
> > - case PTR_TO_MAP_VALUE:
> > - case PTR_TO_MEM:
> > - case PTR_TO_MEM | MEM_RDONLY:
> > - case PTR_TO_MEM | MEM_ALLOC:
> > - case PTR_TO_BUF:
> > - case PTR_TO_BUF | MEM_RDONLY:
> > - case PTR_TO_STACK:
> > - /* Some of the argument types nevertheless require a
> > - * zero register offset.
> > - */
> > - if (arg_type == ARG_PTR_TO_ALLOC_MEM)
> > - goto force_off_check;
> > - break;
> > - /* All the rest must be rejected: */
> > - default:
> > -force_off_check:
> > - err = __check_ptr_off_reg(env, reg, regno,
> > - type == PTR_TO_BTF_ID);
> > - if (err < 0)
> > - return err;
> > - break;
> > - }
> > + err = check_func_arg_reg_off(env, reg, regno, arg_type);
> > + if (err)
> > + return err;
> >
> > skip_type_check:
> > if (reg->ref_obj_id) {
> > --
> > 2.35.1
> >
--
Kartikeya
