On Tue, Dec 5, 2023 at 3:21 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Mon, 2023-12-04 at 15:39 -0800, Andrii Nakryiko wrote:
>
> Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
>
> [...]
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 379ac0a28405..c3a5d0fe3cdf 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -704,6 +704,7 @@ enum bpf_arg_type {
> >
> > ARG_PTR_TO_CTX, /* pointer to context */
> > ARG_ANYTHING, /* any (initialized) argument is ok */
> > + ARG_SCALAR = ARG_ANYTHING, /* scalar value */
>
> Nit: I agree that use of ARG_ANYTHING to denote scalars is confusing,
> but having two names for the same thing seems even more confusing.
>
fair enough, I was undecided on this, I'll revert and use ARG_ANYTHING for now
> [...]
>
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index d56433bf8aba..33a62df9c5a8 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -6955,9 +6955,9 @@ int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog,
> > * 0 - Successfully converted BTF into bpf_reg_state
> > * (either PTR_TO_CTX or SCALAR_VALUE).
> > */
> > -int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
> > - struct bpf_reg_state *regs, u32 *arg_cnt)
> > +int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
>
> Could you please also update the comment above this function?
> It currently says: "Convert BTF of a function into bpf_reg_state if possible".
>
absolutely, will do
> [...]
>
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index ee707736ce6b..16d5550eda4d 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> [...]
> > @@ -19860,33 +19855,44 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
> [...]
> > for (i = BPF_REG_1; i <= BPF_REG_5; i++) {
> > - if (regs[i].type == PTR_TO_CTX)
> > + arg = &sub->args[i - BPF_REG_1];
> > + reg = ®s[i];
> > +
> > + if (arg->arg_type == ARG_PTR_TO_CTX) {
> > + reg->type = PTR_TO_CTX;
> > mark_reg_known_zero(env, regs, i);
> > - else if (regs[i].type == SCALAR_VALUE)
> > + } else if (arg->arg_type == ARG_SCALAR) {
> > + reg->type = SCALAR_VALUE;
> > mark_reg_unknown(env, regs, i);
> > - else if (base_type(regs[i].type) == PTR_TO_MEM) {
> > - const u32 mem_size = regs[i].mem_size;
> > -
> > + } else if (base_type(arg->arg_type) == ARG_PTR_TO_MEM) {
> > + reg->type = PTR_TO_MEM;
> > + if (arg->arg_type & PTR_MAYBE_NULL)
> > + reg->type |= PTR_MAYBE_NULL;
> > mark_reg_known_zero(env, regs, i);
> > - regs[i].mem_size = mem_size;
> > - regs[i].id = ++env->id_gen;
> > + reg->mem_size = arg->mem_size;
> > + reg->id = ++env->id_gen;
> > }
>
> Nit: maybe add an else branch here and report an error if unexpected
> argument type is returned by btf_prepare_func_args()?
true, not sure why I didn't do it here, adding...
>
>
