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

Eduard Zingerman
2026-04-24T22:48:21Z

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.

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.

>
> 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.

[...]

> 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?

> }
>
> 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?

> 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.

[...]

> @@ -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.

[...]

> @@ -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.

> }
>
> 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?

> + }));
> + }
>
> 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.

[...]

> @@ -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.

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