Re: [PATCH bpf-next v3 2/9] bpf: Assign reg->id when getting referenced kptr from ctx

Amery Hung
2026-04-23T22:46:41Z

On Wed, Apr 22, 2026 at 3:50 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Wed, 2026-04-22 at 15:45 -0700, Amery Hung wrote:
> > On Wed, Apr 22, 2026 at 2:46 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > >
> > > On Tue, 2026-04-21 at 15:10 -0700, Amery Hung wrote:
> > > > Assign reg->id when getting referenced kptr from read program context
> > > > to be consistent with R0 of KF_ACQUIRE kfunc. skb dynptr will track the
> > > > referenced skb in qdisc programs using a new field reg->parent_id in
> > > > a later patch.
> > > >
> > > > Signed-off-by: Amery Hung <ameryhung@gmail.com>
> > > > ---
> > > > kernel/bpf/verifier.c | 5 +++--
> > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > > index 41e4ea41c72e..93003a2a96b0 100644
> > > > --- a/kernel/bpf/verifier.c
> > > > +++ b/kernel/bpf/verifier.c
> > > > @@ -6448,8 +6448,6 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
> > > > } else {
> > > > mark_reg_known_zero(env, regs,
> > > > value_regno);
> > > > - if (type_may_be_null(info.reg_type))
> > > > - regs[value_regno].id = ++env->id_gen;
> > > > /* A load of ctx field could have different
> > > > * actual load size with the one encoded in the
> > > > * insn. When the dst is PTR, it is for sure not
> > > > @@ -6459,8 +6457,11 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
> > > > if (base_type(info.reg_type) == PTR_TO_BTF_ID) {
> > > > regs[value_regno].btf = info.btf;
> > > > regs[value_regno].btf_id = info.btf_id;
> > > > + regs[value_regno].id = info.ref_obj_id;
> > > > regs[value_regno].ref_obj_id = info.ref_obj_id;
> > > > }
> > >
> > > Hi Amery,
> > >
> > > Could you please help me double-check my understanding of this change?
> > >
> > > It adds a new behavior, if struct_ops method parameter is annotated
> > > with __ref, then if a field is read from such struct_ops context
> > > the destination register would now have it's .id == ref_obj_id
> > > allocated for the parameter at the beginning of do_check().
> > > E.g. for `rB = *(T *)(rA + off)`, if `rA` is a pointer to struct_ops
> > > context and parameter corresponding to `off` has `__ref` annotation
> > > the `rB.id` will be assigned, contrary to old behavior, when it remained 0.
> > >
> >
> > Right
>
> Ack, thank you.
>
> > > This change in behavior, however, becomes visible only after the
> > > changes to release_reference() later in the series.
> > > There the `rB.id` is used to initiate `mark_reg_invalid(env, reg)` for
> > > the `rB`.
> > >
> >
> > I probably should mention it in the commit message. The purpose of the
> > patch is to make the implementation consistent with the design:
> > parent_id tracks the id of the parent objects.
> >
> > Without this patch, it should still work since in this case ref_obj_id
> > always equals to id. When release_reference() is called on a
> > KF_RELEASE kfunc, the children objects will still be invalidated
> > correctly.
>
> bpf_qdisc_fail__invalid_dynptr_cross_frame.c:invalid_dynptr_cross_frame
> fails if I comment out the line `regs[value_regno].id = info.ref_obj_id;`.
> The added .id link is used to propagate invalidation triggered by
> `free_skb(skb);` to `hdr`.
> The patch seem mandatory for the series.

Ah yes... because an skb dynptr do track parent's id in
check_kfunc_args(). Thanks for the correction.

ret = process_dynptr_func(env, regno, insn_idx, dynptr_arg_type,
meta->ref_obj_id ? meta->id : 0, &meta->dynptr);