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

Andrii Nakryiko
2026-04-24T00:04:56Z

On Thu, Apr 23, 2026 at 11:44 AM Amery Hung <ameryhung@gmail.com> wrote:
>
> On Thu, Apr 23, 2026 at 11:19 AM Mykyta Yatsenko
> <mykyta.yatsenko5@gmail.com> wrote:
> >
> >
> >
> > On 4/21/26 11:10 PM, Amery Hung wrote:
> > > Refactor object relationship tracking in the verifier and fix a dynptr
> > > use-after-free bug where file/skb dynptrs are not invalidated when the
> > > parent referenced object is freed.
> > >
> > > Add parent_id to bpf_reg_state to precisely track child-parent
> > > relationships. A child object's parent_id points to the parent object's
> > > id. This replaces the PTR_TO_MEM-specific dynptr_id and does not
> > > increase the size of bpf_reg_state on 64-bit machines as there is
> > > existing padding.
> > >
> > > When calling dynptr constructors (i.e., process_dynptr_func() with
> > > MEM_UNINIT argument), track the parent's id if the parent is a
> > > referenced object. This only applies to file dynptr and skb dynptr,
> > > so only pass parent reg->id to kfunc constructors.
> > >
> > > For release_reference(), invalidating an object now also invalidates
> > > all descendants by traversing the object tree. This is done using
> > > stack-based DFS to avoid recursive call chains of release_reference() ->
> > > unmark_stack_slots_dynptr() -> release_reference(). Referenced objects
> > > encountered during tree traversal cannot be indirectly released. They
> > > require an explicit helper/kfunc call to release the acquired resources.
> > >
> > > While the new design changes how object relationships are tracked in
> > > the verifier, it does not change the verifier's behavior. Here is the
> > > implication for dynptr, pointer casting, and owning/non-owning
> > > references:
> > >
> > > Dynptr:
> > >
> > > When initializing a dynptr, referenced dynptrs acquire a reference for
> > > ref_obj_id. If the dynptr has a referenced parent, parent_id tracks the
> > > parent's id. When cloning, ref_obj_id and parent_id are copied from the
> > > original. Releasing a referenced dynptr via release_reference(ref_obj_id)
> > > invalidates all clones and derived slices. For non-referenced dynptrs,
> > > only the specific dynptr and its children are invalidated.
> > >
> > > Pointer casting:
> > >
> > > Referenced socket pointers and their casted counterparts share the same
> > > lifetime but have different nullness — they have different id but the
> > > same ref_obj_id.
> > >
> > > Owning to non-owning reference conversion:
> > >
> > > After converting owning to non-owning by clearing ref_obj_id (e.g.,
> > > object(id=1, ref_obj_id=1) -> object(id=1, ref_obj_id=0)), the
> > > verifier only needs to release the reference state, so it calls
> > > release_reference_nomark() instead of release_reference().
> > >
> > > Note that the error message "reference has not been acquired before" in
> > > the helper and kfunc release paths is removed. This message was already
> > > unreachable. The verifier only calls release_reference() after
> > > confirming meta.ref_obj_id is valid, so the condition could never
> > > trigger in practice (no selftest exercises it either). With the
> > > refactor, release_reference() can now be called with non-acquired ids
> > > and have different error conditions. Report directly in
> > > release_reference() instead.
> > >
> > > Fixes: 870c28588afa ("bpf: net_sched: Add basic bpf qdisc kfuncs")
> > > Signed-off-by: Amery Hung <ameryhung@gmail.com>
> > > ---
> > > include/linux/bpf_verifier.h | 22 ++-
> > > kernel/bpf/log.c | 4 +-
> > > kernel/bpf/states.c | 9 +-
> > > kernel/bpf/verifier.c | 264 +++++++++++++++++------------------
> > > 4 files changed, 152 insertions(+), 147 deletions(-)
> > >

[...]

> > > @@ -673,82 +675,56 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_
> > > mark_dynptr_stack_regs(env, &state->stack[spi].spilled_ptr,
> > > &state->stack[spi - 1].spilled_ptr, type);
> > >
> > > - if (dynptr_type_refcounted(type)) {
> > > - /* The id is used to track proper releasing */
> > > - int id;
> > > -
> > > - if (clone_ref_obj_id)
> > > - id = clone_ref_obj_id;
> > > - else
> > > - id = acquire_reference(env, insn_idx);
> > > -
> > > - if (id < 0)
> > > - return id;
> > > -
> > > - state->stack[spi].spilled_ptr.ref_obj_id = id;
> > > - state->stack[spi - 1].spilled_ptr.ref_obj_id = id;

is id assigned somewhere else now? where?

> > > + if (dynptr->type == BPF_DYNPTR_TYPE_INVALID) { /* dynptr constructors */
> > > + if (dynptr_type_referenced(type)) {
> > > + ref_obj_id = acquire_reference(env, insn_idx);
> > > + if (ref_obj_id < 0)
> > > + return ref_obj_id;
> > > + }
> > > + } else { /* bpf_dynptr_clone() */
> > > + ref_obj_id = dynptr->ref_obj_id;
> > > + parent_id = dynptr->parent_id;
> > > }
> > >
> > > + state->stack[spi].spilled_ptr.ref_obj_id = ref_obj_id;
> > > + state->stack[spi - 1].spilled_ptr.ref_obj_id = ref_obj_id;
> > > + state->stack[spi].spilled_ptr.parent_id = parent_id;
> > > + state->stack[spi - 1].spilled_ptr.parent_id = parent_id;
> > > +
> > > return 0;
> > > }
> > >

[...]

> > > -/* The pointer with the specified id has released its reference to kernel
> > > - * resources. Identify all copies of the same pointer and clear the reference.
> > > - *
> > > - * This is the release function corresponding to acquire_reference(). Idempotent.
> > > - */
> > > -static int release_reference(struct bpf_verifier_env *env, int ref_obj_id)
> > > +static int idstack_push(struct bpf_idmap *idmap, u32 id)
> > > +{
> > > + int i;
> > > +
> > > + if (!id)
> > > + return 0;
> > > +
> > > + for (i = 0; i < idmap->cnt; i++)
> > > + if (idmap->map[i].old == id)
> > > + return 0;
> > > +
> > > + if (WARN_ON_ONCE(idmap->cnt >= BPF_ID_MAP_SIZE))
> > > + return -EFAULT;
> >
> > It feels like this check belongs above, maybe the first thing to do.
>
> Shouldn't it allow the verification to continue when pushing an id
> that already exists in the stack?

yep

>
> >
> > > +
> > > + idmap->map[idmap->cnt++].old = id;
> > > + return 0;
> > > +}
> > > +
> > > +static int idstack_pop(struct bpf_idmap *idmap)
> > > {
> > > + if (!idmap->cnt)
> > > + return 0;
> > > +
> > > + return idmap->map[--idmap->cnt].old;
> > > +}
> > > +
> > > +/* 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) {
> >
> > Does this line check that the only ref_obj_id we should see is either
> > 0 or root_id? can we rewrite it as
> > if (reg->ref_obj_id && reg->ref_obj_id != root_id)
> >
> > this is simpler, because id can also be reg->id/reg->parent_id, which is
> > hard to reason what that means.
> >
>
> Looking at it again, I think my release_reference() is wrong. It first
> calls release_reference_nomark() if the id is a ref_obj_id. This will
> invalidate all registers with the same ref_obj_id without traversing
> their children.
>

hm, will it? from what I see, release_reference_nomark() doesn't do
anything with registers/stack slots, just removes the reference from
state->refs

> I should remember the root_ref_obj_id instead of calling
> release_reference_nomark(). Then, the check you mentioned should be:
> if (reg->ref_obj_id && reg->ref_obj_id != root_ref_obj_id)
>
> I will write a selftest to confirm the issue and include it in the
> next respin if this turns out to be a bug.