On Fri, Mar 6, 2026 at 10:44 PM Amery Hung [off-list ref] wrote:

Refactor object relationship tracking in the verifier by removing
dynptr_id and using parent_id to track the parent object. Then, track
the referenced parent object for the dynptr when calling a dynptr
constructor. This fixes a use-after-free bug. For dynptr that has
referenced parent object (skb dynptr in BPF qdisc or file dynptr),
the dynptr or derived slices need to be invalidated when the parent
object is released.

First, add parent_id to bpf_reg_state to be able to precisely track
objects' child-parent relationship. A child object will use parent_id
to track the parent object's id. This replaces dynptr slice specific
dynptr_id.

Then, when calling dynptr constructors (i.e., process_dynptr_func() with
MEM_UNINIT argument), track the parent's id if parent is an referenced
object. This only applies to file dynptr and skb dynptr, so only pass
parent reg->id to kfunc constructors.

For release_reference(), this mean when invalidating an object, it needs
to also invalidate all dependent objects by traversing the subtree. This
is done using stack-based DFS to avoid recursive call chain of
release_reference() -> unmark_stack_slots_dynptr() ->
release_reference(). Note that, referenced objects cannot be released
when traversing the tree if it is not the object id initially passed to
release_reference() as they would actually require helper call to
release the acquired resources.

While the new design changes how object relationships are being tracked
in the verifier, it does NOT change the verifier's behavior. Here is
the implication of the new design for dynptr, ptr casting and
owning/non-owning references.

Dynptr:

When initializing a dynptr, referenced dynptr will acquire an reference
for ref_obj_id. If the dynptr has a referenced parent, the parent_id
will be used to track the its id. When cloning dynptr, ref_obj_id and
parent_id of the clone are copied directly from the original dynptr.
This means, when releasing a dynptr, if it is a referenced dynptr,
release_reference(ref_obj_id) will release all clones and the original
and derived slices. For non-referenced dynptr, only the specific dynptr
being released and its children slices will be invalidated.

Pointer casting:

Referenced socket pointer and the casted pointers should share the same
lifetime, while having difference nullness. Therefore, they will have
different id but the same ref_obj_id.

When converting owning references to non-owning:

After converting a reference from owning to non-owning by clearing the
object's ref_reg_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
instead of releasing registers that have the id, so call
release_reference_nomark() instead of release_reference().

CC: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Fixes: 870c28588afa ("bpf: net_sched: Add basic bpf qdisc kfuncs")
Signed-off-by: Amery Hung <redacted>
---
 include/linux/bpf_verifier.h |  14 +-
 kernel/bpf/log.c             |   4 +-
 kernel/bpf/verifier.c        | 274 ++++++++++++++++++-----------------
 3 files changed, 154 insertions(+), 138 deletions(-)

[...]

-       ref_obj_id = state->stack[spi].spilled_ptr.ref_obj_id;
-
-       /* If the dynptr has a ref_obj_id, then we need to invalidate
-        * two things:
-        *
-        * 1) Any dynptrs with a matching ref_obj_id (clones)
-        * 2) Any slices derived from this dynptr.
+       /*
+        * For referenced dynptr, the clones share the same ref_obj_id and will be
+        * invalidated too. For non-referenced dynptr, only the dynptr and slices
+        * derived from it will be invalidated.
         */

this is confusing to me. Why the nature of dynptr should change
anything about the scope of invalidation? This should be controlled
from outside. E.g., if someone invalidates clone by overwriting it on
the stack, we shouldn't just go an invalidate all other clones. We
just invalidate that particular clone (regardless of whether it's a
clone of file dynptr or just some mem dynptr).

But if someone is calling bpf_dynptr_file_discard() on one of the
clones, then yes, all the clones need to be invalidated. But that
should be handled as more generic "this file lifetime is ending", no?

Maybe I'm missing something, but it feels wrong to make decisions like
this inside a low-level (and thus intentionally dumb)
unmark_stack_slots_dynptr() helper.

-
-       /* Invalidate any slices associated with this dynptr */
-       WARN_ON_ONCE(release_reference(env, ref_obj_id));
-
-       /* Invalidate any dynptr clones */
-       for (i = 1; i < state->allocated_stack / BPF_REG_SIZE; i++) {
-               if (state->stack[i].spilled_ptr.ref_obj_id != ref_obj_id)
-                       continue;

[...]

+static u32 idstack_pop(struct bpf_idstack *idstack)
+{
+       return idstack->cnt > 0 ? idstack->ids[--idstack->cnt] : 0;
+}
+
+static int release_reg_check(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
+                            int id, int root_id, struct bpf_idstack *idstack)

tbh, I feel like release_reg_check is doing too much, it both enqueues
children and checks unreleased references. And this <0, 0, and 1 as
return values (where 1 is completely unobvious) is an indicator of
that. I think id/parent_id/ref_obj_id check can be done inline in
release_reg_check() just fine (yes, two place, no big deal, but if so,
make it a small helper) and then you'll have more obvious logic break
down into a) check if reg should be enqueue, b) if so, check ref leak,
and c) enqueue new id

 {
+       struct bpf_reference_state *ref_state;
+
+       if (reg->id == id || reg->parent_id == id || reg->ref_obj_id == id) {
+               /* Cannot indirectly release a referenced id */
+               if (reg->ref_obj_id && id != root_id) {
+                       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 && reg->id != id)
+                       idstack_push(idstack, reg->id);

can't you push the same id multiple times into stack this way? your
idstack is actually a set, no? so idmap serves you better (just map id
to 1 for "to be checked")? And then you don't need to introduce a new
idstack_scratch data structure

+               return 1;
+       }
+
+       return 0;
+}
+

[...]