Re: [PATCH bpf-next v2 5/6] selftests/bpf: Add tests for pointer type merge at a shared load

[email protected]
Newsgroups org.kernel.vger.bpf
Message-ID <51afcc2b1c0704ce7cec1e6ff7aa6423fee6b3afad0ceebc4a118828ea87586b@mail.kernel.org>
> diff --git a/tools/testing/selftests/bpf/progs/mem_rdonly_untrusted.c b/tools/testing/selftests/bpf/progs/mem_rdonly_untrusted.c
> index b91271d4caa4..ac8bba3e7d02 100644
> --- a/tools/testing/selftests/bpf/progs/mem_rdonly_untrusted.c
> +++ b/tools/testing/selftests/bpf/progs/mem_rdonly_untrusted.c

[ ... ]

> +SEC("socket")
> +__success
> +__log_level(2)
> +__msg("= *(u64 *)(r{{[0-9]}} +0){{.*}}=untrusted_ptr_sock")
> +__msg("= *(u64 *)(r{{[0-9]}} +0){{.*}}=ringbuf_mem")
> +__retval(0)
> +int mixed_mem_untrusted_btf_id_type(void *ctx)

[ ... ]

> +SEC("socket")
> +__success
> +__log_level(2)
> +__msg("= *(u32 *)(r{{[0-9]}} +0){{.*}}=ptr_nameidata")
> +__msg("= *(u32 *)(r{{[0-9]}} +0){{.*}}=ringbuf_mem")
> +__retval(0)
> +int mixed_mem_btf_id_type(void *ctx)

[ ... ]

> +SEC("socket")
> +__success
> +__log_level(2)
> +__msg("r8 = *(u32 *)(r7 +0){{.*}}R7=ptr_nameidata")
> +__msg("r8 = *(u32 *)(r7 +0){{.*}}R7=rdonly_mem")
> +__retval(0)
> +int mixed_rdonly_mem_btf_id_type(void *ctx)

[ ... ]

> +SEC("socket")
> +__success
> +__log_level(2)
> +__msg("= *(u64 *)(r{{[0-9]}} +0){{.*}}=ringbuf_mem")
> +__msg("= *(u64 *)(r{{[0-9]}} +0){{.*}}=rdonly_untrusted_mem")
> +__retval(0)
> +int mixed_mem_mem_type(void *ctx)

[ ... ]

> +SEC("socket")
> +__failure
> +__msg("same insn cannot be used with different pointers")
> +int mixed_map_value_mem_type(void *ctx)

[ ... ]

> +SEC("socket")
> +__failure
> +__msg("same insn cannot be used with different pointers")
> +int mixed_stack_mem_type(void *ctx)

[ ... ]

A subsystem pattern flags this as potentially concerning: these six
tests cover only two of the four commits in the series. The three merge
shapes listed in dea41ba6e67d's changelog map 1:1 onto the
mixed_mem_untrusted_btf_id_type, mixed_mem_btf_id_type, and
mixed_rdonly_mem_btf_id_type tests, and the mixed_mem_mem_type,
mixed_map_value_mem_type, and mixed_stack_mem_type tests cover the new
PTR_TO_MEM mismatch rule from fbffa57329b8.

However, the other two commits appear untested:

bcfd0d6a160f changed check_ptr_to_btf_access() from checking
type_flag(reg->type) & PTR_UNTRUSTED to calling
bpf_may_fault_on_deref(), so a BPF_STX through a bare PTR_TO_BTF_ID is
now rejected with "only read is supported". The changelog documents a
reachable bpf_qdisc panic (next = sch->next_sched; next->limit = 1000;),
but no __failure / __msg("only read is supported") case is added here or
in progs/bpf_qdisc_*.c. Only verifier_global_ptr_args.c and
verifier_netfilter_ctx.c assert that message, and both do so for
PTR_UNTRUSTED pointers that the old check already caught, so the
newly-closed hole is untested.

bef47cf76444 states that PTR_TO_BTF_ID|PTR_UNTRUSTED|MEM_ALLOC|NON_OWN_REF
and PTR_TO_BTF_ID|PTR_UNTRUSTED|MEM_RCU were reachable and left as plain
loads (two Fixes: tags). The added tests only produce
ringbuf/rdonly/bare-BTF-ID types, all of which the old exact-match list
in bpf_convert_ctx_accesses() already handled or which the merge path
covers.

Would it make sense to add tests for the bcfd0d6a160f store-rejection path
and the bef47cf76444 MEM_ALLOC|NON_OWN_REF and MEM_RCU cases here?


---
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/31845722500
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.