Re: [PATCH bpf-next v4 03/12] bpf: Assign reg->id when getting referenced kptr from ctx

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 3173 bytes --]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -6057,8 +6057,6 @@ struct bpf_insn_access_aux info = {
>  			} 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
> @@ -6068,8 +6066,11 @@ struct bpf_insn_access_aux info = {
>  				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;
                                        ^^^^

Does this introduce a bug when reading the same nullable context field
multiple times?

The new code sets reg->id = info.ref_obj_id, where ref_obj_id comes from
ctx_arg_info->ref_obj_id. The ctx_arg_info structure is initialized once
during program type setup in check_attach_btf_id(), making ref_obj_id a
static value per context field, not per read.

When a BPF program reads the same nullable context field into different
registers, they would share the same ID:

    r1 = ctx->nullable_kptr;  // r1->id = ref_obj_id = 5
    r2 = ctx->nullable_kptr;  // r2->id = ref_obj_id = 5 (SAME!)
    if (r1 != NULL) {
        // mark_ptr_or_null_regs(id=5) marks ALL registers with id==5
        // Both r1 AND r2 are now considered safe
        use(r2);  // BUG: r2 was never checked but verifier allows use
    }

The mark_ptr_or_null_reg() function uses reg->id to identify which
registers should be updated together after a NULL check. With shared IDs,
independent reads are incorrectly treated as aliases.

The old code generated a unique ID via ++env->id_gen for each nullable
read, ensuring proper NULL-check tracking.

>  					regs[value_regno].ref_obj_id = info.ref_obj_id;
>  				}
> +				if (type_may_be_null(info.reg_type) && !regs[value_regno].id)
> +					regs[value_regno].id = ++env->id_gen;

Would this fallback path prevent fresh ID generation when id was already
set to ref_obj_id? The condition '!regs[value_regno].id' is now false for
nullable PTR_TO_BTF_ID pointers, so each read gets the same static ID
instead of a unique one.

Eduard Zingerman's review comment from v3 suggested that the commit message
should mention that this patch is mandatory for proper invalidation
propagation in later patches. He noted that without the line
'regs[value_regno].id = info.ref_obj_id;', the test
'bpf_qdisc_fail__invalid_dynptr_cross_frame.c:invalid_dynptr_cross_frame'
fails because the .id link is needed to propagate invalidation triggered by
'free_skb(skb);' to 'hdr'. The current commit message mentions consistency
and parent_id tracking but does not explicitly mention the invalidation
propagation aspect.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/25442456702