Re: [PATCH bpf-next v3 4/9] bpf: Refactor object relationship tracking and fix dynptr UAF bug

Amery Hung
2026-04-27T20:22:08Z

On Fri, Apr 24, 2026 at 3:48 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Tue, 2026-04-21 at 15:10 -0700, Amery Hung wrote:
>
> Tbh, I find current state of affairs with id/ref_obj_id/parent_id hard
> to follow. The release_reference() is an improvement, but the means by
> which the fields are propagated to bpf_reg_state objects are convoluted.
> I wonder if having a separate "object table" in bpf_verifier_state and
> having bpf_reg_state->id refer to objects within this table would make
> things more straight forward.

I think this is a good idea in the long term. First, we need to make
id a stable and unique object identifier (i.e., always assign id to
objects; different objects have different ids). Then, we can create
the object table indexed by id and each entry contains the ref_obj_id
and parent_id moved from bpf_reg_state. Then, there will be helpers
managing the lifetime, relationship (e.g., bpf_obj_create,
bpf_obj_release, bpf_obj_clone) with clear semantics to centralize how
id,parent_id,ref_obj_id are manipulated.

If this makes sense, I can send this as a follow up patchset.

>
> A few nits below.
>
> [...]
>
> > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
>
> [...]
>
> > @@ -1323,6 +1335,7 @@ struct bpf_dynptr_desc {
> > enum bpf_dynptr_type type;
> > u32 id;
> > u32 ref_obj_id;
> > + u32 parent_id;
> > };
>
> Nit: would be nice to have a comment describing when the above fields
> are populated.

Ack

>
> >
> > struct bpf_kfunc_call_arg_meta {
> > @@ -1334,6 +1347,7 @@ struct bpf_kfunc_call_arg_meta {
> > const char *func_name;
> > /* Out parameters */
> > u32 ref_obj_id;
> > + u32 id;
>
> Nit: would be nice to have a comment here too.

Ack

>
> [...]
>
> > diff --git a/kernel/bpf/states.c b/kernel/bpf/states.c
> > index 8478d2c6ed5b..72bd3bcda5fb 100644
> > --- a/kernel/bpf/states.c
> > +++ b/kernel/bpf/states.c
> > @@ -494,7 +494,8 @@ static bool regs_exact(const struct bpf_reg_state *rold,
> > {
> > return memcmp(rold, rcur, offsetof(struct bpf_reg_state, id)) == 0 &&
> > check_ids(rold->id, rcur->id, idmap) &&
> > - check_ids(rold->ref_obj_id, rcur->ref_obj_id, idmap);
> > + check_ids(rold->ref_obj_id, rcur->ref_obj_id, idmap) &&
> > + check_ids(rold->parent_id, rcur->parent_id, idmap);
>
> Nit: these check_ids() become repetitive, maybe add a utility function
> checking id/ref_obj_id/parent_id?

Will add a utility function.

>
> > }
> >
> > enum exact_level {
> > @@ -619,7 +620,8 @@ static bool regsafe(struct bpf_verifier_env *env, struct bpf_reg_state *rold,
> > range_within(rold, rcur) &&
> > tnum_in(rold->var_off, rcur->var_off) &&
> > check_ids(rold->id, rcur->id, idmap) &&
> > - check_ids(rold->ref_obj_id, rcur->ref_obj_id, idmap);
> > + check_ids(rold->ref_obj_id, rcur->ref_obj_id, idmap) &&
> > + check_ids(rold->parent_id, rcur->parent_id, idmap);
> > case PTR_TO_PACKET_META:
> > case PTR_TO_PACKET:
> > /* We must have at least as much range as the old ptr
> > @@ -799,7 +801,8 @@ static bool stacksafe(struct bpf_verifier_env *env, struct bpf_func_state *old,
> > cur_reg = &cur->stack[spi].spilled_ptr;
> > if (old_reg->dynptr.type != cur_reg->dynptr.type ||
> > old_reg->dynptr.first_slot != cur_reg->dynptr.first_slot ||
> > - !check_ids(old_reg->ref_obj_id, cur_reg->ref_obj_id, idmap))
> > + !check_ids(old_reg->ref_obj_id, cur_reg->ref_obj_id, idmap) ||
> > + !check_ids(old_reg->parent_id, cur_reg->parent_id, idmap))
>
> Not something changed by the current patch, but still a question:
> this path ignores old_reg->id, is it a bug?

Yes. I will also check id.

>
> > return false;
> > break;
> > case STACK_ITER:
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 0313b7d5f6c9..908a3af0e7c4 100644
>
> [...]
>
> > @@ -241,6 +241,7 @@ struct bpf_call_arg_meta {
> > int mem_size;
> > u64 msize_max_value;
> > int ref_obj_id;
> > + u32 id;
>
> Nit: please add a comment here as well.

Ack

>
> [...]
>
> > @@ -635,11 +636,12 @@ static int destroy_if_dynptr_stack_slot(struct bpf_verifier_env *env,
> > struct bpf_func_state *state, int spi);
> >
> > static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> > - enum bpf_arg_type arg_type, int insn_idx, int clone_ref_obj_id)
> > + enum bpf_arg_type arg_type, int insn_idx, int parent_id,
> > + struct bpf_dynptr_desc *dynptr)
>
> Having both parent_id and dynptr->parent_id as parameters of this
> function is very confusing, but I don't have a suggestion on how to
> better deal with it.

Does tweaking the argument name and reorder the if-else block make it
more obvious how these two arguments are used?

if (clone_dynptr->type != BPF_DYNPTR_TYPE_INVALID) { /* bpf_dynptr_clone() */
ref_obj_id = clone_dynptr->ref_obj_id;
parent_id = clone_dynptr->parent_id;
} else { /* dynptr constructors */
if (dynptr_type_referenced(type)) {
ref_obj_id = acquire_reference(env, insn_idx);
if (ref_obj_id < 0)
return ref_obj_id;
}
}

>
> [...]
>
> > @@ -8489,6 +8444,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> > return -EACCES;
> > }
> > meta->ref_obj_id = reg->ref_obj_id;
> > + meta->id = reg->id;
>
> Could you please leave a comment here explaining when e.g.
> meta->ref_obj_id and meta->id would have same or different ids here?
> Maybe a few lines of BPF C code.

Ack

>
> > }
> >
> > switch (base_type(arg_type)) {
> > @@ -9111,26 +9067,75 @@ static int release_reference_nomark(struct bpf_verifier_state *state, int ref_ob
>
> [...]
>
> > +/* Release id and objects referencing the id iteratively in a DFS manner */
> > +static int release_reference(struct bpf_verifier_env *env, int id)
> > +{
> > + u32 mask = (1 << STACK_SPILL) | (1 << STACK_DYNPTR);
> > struct bpf_verifier_state *vstate = env->cur_state;
> > + struct bpf_idmap *idstack = &env->idmap_scratch;
> > + struct bpf_stack_state *stack;
> > struct bpf_func_state *state;
> > struct bpf_reg_state *reg;
> > - int err;
> > + int root_id = id, err;
> >
> > - err = release_reference_nomark(vstate, ref_obj_id);
> > - if (err)
> > - return err;
> > + idstack->cnt = 0;
> > + idstack_push(idstack, id);
> >
> > - bpf_for_each_reg_in_vstate(vstate, state, reg, ({
> > - if (reg->ref_obj_id == ref_obj_id)
> > - mark_reg_invalid(env, reg);
> > - }));
> > + if (find_reference_state(vstate, id))
> > + WARN_ON_ONCE(release_reference_nomark(vstate, id));
> > +
> > + while ((id = idstack_pop(idstack))) {
> > + bpf_for_each_reg_in_vstate_mask(vstate, state, reg, stack, mask, ({
> > + if (reg->id != id && reg->parent_id != id && reg->ref_obj_id != id)
> > + continue;
> > +
> > + if (reg->ref_obj_id && id != root_id) {
> > + struct bpf_reference_state *ref_state;
> > +
> > + ref_state = find_reference_state(env->cur_state, reg->ref_obj_id);
> > + verbose(env, "Unreleased reference id=%d alloc_insn=%d when releasing id=%d\n",
> > + ref_state->id, ref_state->insn_idx, root_id);
> > + return -EINVAL;
> > + }
> > +
> > + if (reg->id != id) {
> > + err = idstack_push(idstack, reg->id);
> > + if (err)
> > + return err;
> > + }
> > +
> > + if (!stack || stack->slot_type[BPF_REG_SIZE - 1] == STACK_SPILL)
> > + mark_reg_invalid(env, reg);
> > + else if (stack->slot_type[BPF_REG_SIZE - 1] == STACK_DYNPTR)
> > + invalidate_dynptr(env, state, stack);
>
> invalidate_dynptr() rewrites to stack slots, can it be the case that
> this body of bpf_for_each_reg_in_vstate_mask() is computed for first
> and second dynptr stack slots, hence triggering invalidate_dynptr() to
> rewrite three slots instead of two?

invalidate_dynptr() will mark the two stack slots as STACK_INVALID so
I didn't bother to check whether it is the first slot or not. Am I
missing anything?

>
> > + }));
> > + }
> >
> > return 0;
> > }
>
> [...]
>
> > @@ -12009,6 +12009,7 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
> > return -EFAULT;
> > }
> > meta->ref_obj_id = reg->ref_obj_id;
> > + meta->id = reg->id;
>
> And here a comment describing when this happens would be helpful.
>

Ack

> [...]
>
> > @@ -12171,15 +12171,10 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
> > }
> >
> > dynptr_arg_type |= (unsigned int)get_dynptr_type_flag(parent_type);
> > - clone_ref_obj_id = meta->dynptr.ref_obj_id;
> > - if (dynptr_type_refcounted(parent_type) && !clone_ref_obj_id) {
> > - verifier_bug(env, "missing ref obj id for parent of clone");
> > - return -EFAULT;
> > - }
> > }
> >
> > - ret = process_dynptr_func(env, regno, insn_idx, dynptr_arg_type, clone_ref_obj_id,
> > - &meta->dynptr);
> > + ret = process_dynptr_func(env, regno, insn_idx, dynptr_arg_type,
> > + meta->ref_obj_id ? meta->id : 0, &meta->dynptr);
>
> And an example here as well.

Ack

>
> > if (ret < 0)
> > return ret;
> > break;
>
> …