On Fri, Feb 14, 2025 at 6:48 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Fri, Feb 14, 2025 at 8:45 AM Amery Hung <ameryhung@xxxxxxxxx> wrote:
> >
> > + if (!is_nullable && !is_refcounted)
> > continue;
> >
> > + if (is_nullable)
> > + suffix = MAYBE_NULL_SUFFIX;
> > + else if (is_refcounted)
> > + suffix = REFCOUNTED_SUFFIX;
>
> I would remove the first 'if' and add:
> else
> continue;
That looks better than what it is. I will fix it.
>
> > /* Should be a pointer to struct */
> > pointed_type = btf_type_resolve_ptr(btf,
> > args[arg_no].type,
> > @@ -236,7 +246,7 @@ static int prepare_arg_info(struct btf *btf,
> > if (!pointed_type ||
> > !btf_type_is_struct(pointed_type)) {
> > pr_warn("stub function %s has %s tagging to an unsupported type\n",
> > - stub_fname, MAYBE_NULL_SUFFIX);
> > + stub_fname, suffix);
> > goto err_out;
> > }
> >
> > @@ -254,11 +264,15 @@ static int prepare_arg_info(struct btf *btf,
> > }
> >
> > /* Fill the information of the new argument */
> > - info->reg_type =
> > - PTR_TRUSTED | PTR_TO_BTF_ID | PTR_MAYBE_NULL;
> > info->btf_id = arg_btf_id;
> > info->btf = btf;
> > info->offset = offset;
> > + if (is_nullable) {
> > + info->reg_type = PTR_TRUSTED | PTR_TO_BTF_ID | PTR_MAYBE_NULL;
> > + } else if (is_refcounted) {
> > + info->reg_type = PTR_TRUSTED | PTR_TO_BTF_ID;
> > + info->refcounted = true;
> > + }
> >
> > info++;
> > info_cnt++;
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 9de6acddd479..fd3470fbd144 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -6677,6 +6677,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> > info->reg_type = ctx_arg_info->reg_type;
> > info->btf = ctx_arg_info->btf ? : btf_vmlinux;
> > info->btf_id = ctx_arg_info->btf_id;
> > + info->ref_obj_id = ctx_arg_info->refcounted ? ctx_arg_info->ref_obj_id : 0;
> > return true;
> > }
> > }
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index a41ba019780f..a0f51903e977 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -1543,6 +1543,17 @@ static void release_reference_state(struct bpf_verifier_state *state, int idx)
> > return;
> > }
> >
> > +static bool find_reference_state(struct bpf_verifier_state *state, int ptr_id)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < state->acquired_refs; i++)
> > + if (state->refs[i].id == ptr_id)
> > + return true;
> > +
> > + return false;
> > +}
> > +
> > static int release_lock_state(struct bpf_verifier_state *state, int type, int id, void *ptr)
> > {
> > int i;
> > @@ -5981,7 +5992,8 @@ static int check_packet_access(struct bpf_verifier_env *env, u32 regno, int off,
> > /* check access to 'struct bpf_context' fields. Supports fixed offsets only */
> > static int check_ctx_access(struct bpf_verifier_env *env, int insn_idx, int off, int size,
> > enum bpf_access_type t, enum bpf_reg_type *reg_type,
> > - struct btf **btf, u32 *btf_id, bool *is_retval, bool is_ldsx)
> > + struct btf **btf, u32 *btf_id, bool *is_retval, bool is_ldsx,
> > + u32 *ref_obj_id)
>
> It's ok-ish for now, but 11 arguments in a function is pushing it.
> We've accumulated enough tech debt here.
> Consider a follow up.
Definitely.
A quick look at the check_ctx_access(): Many arguments are used as
input+output at the same time and are part of bpf_insn_access_aux, so
we might as well just let the caller prepare bpf_insn_access_aux. This
should reduce 5 arguments already.
I will send a follow up patch to cleanup this part.
