Re: [PATCH bpf-next v1 3/3] selftests/bpf: Test PROBE_MEM loads from invalid addresses

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/bpf: shift-count overflow and skeleton layout mismatch on 32-bit hosts
--

commit 96530723e8030fb7a1b5acdb7814f68def3240b2
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

selftests/bpf: Test PROBE_MEM loads from invalid addresses

This commit adds a BPF selftest that verifies whether PROBE_MEM loads from
various invalid addresses properly yield zero and prevent the kernel from
crashing. It does this by testing loads of three sizes through a pointer
value chosen by userspace.

> diff --git a/tools/testing/selftests/bpf/prog_tests/probe_mem_fault.c b/tools/testing/selftests/bpf/prog_tests/probe_mem_fault.c
> new file mode 100644
> index 0000000000000..57a313e35eb62
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/probe_mem_fault.c
> @@ -0,0 +1,69 @@

[ ... ]

> +/*
> + * Addresses a PROBE_MEM load has to survive. Either the JIT's address check
> + * or the fault handler must turn each load into a zero result.
> + */
> +static const struct {
> +	const char *name;
> +	unsigned long addr;

[Severity: Medium]
On 32-bit architectures, unsigned long is 32 bits wide. Could this truncate
64-bit addresses and cause a misalignment of the BSS structure on those
platforms?

> +} bad_addrs[] = {
> +	{ "null", 0 },
> +	{ "low_user", 4096 },
> +	{ "last_user_page", (1UL << 47) - 4096 },

[Severity: Medium]
Will shifting a 32-bit 1UL by 47 bits trigger a -Wshift-count-overflow
compiler warning on 32-bit architectures? Under -Werror, this would cause
a build failure.

Could the ULL suffix be used here instead?

> +	{ "non_canonical", 1UL << 63 },

[Severity: Medium]
Does the same shift-count overflow issue occur here on 32-bit platforms when
shifting 1UL by 63 bits?

> +#if defined(__x86_64__)
> +	{ "vsyscall", VSYSCALL_ADDR },

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/probe_mem_fault.c b/tools/testing/selftests/bpf/progs/probe_mem_fault.c
> new file mode 100644
> index 0000000000000..748be45418f5a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/probe_mem_fault.c
> @@ -0,0 +1,41 @@

[ ... ]

> +int target_pid;
> +bool use_current_task;
> +unsigned long addr;

[Severity: Medium]
If this 8-byte BPF variable is declared as unsigned long, does bpftool emit a
4-byte struct field when building on 32-bit hosts?

If so, this appears to misalign the skeleton bss layout and could cause
subsequent fields like val_dw to be corrupted during runtime reads. Would
__u64 be a safer type here?

> +int runs;
> +__u64 val_dw;
> +__u32 val_w;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009024925.3169077-1-memxor@gmail.com?part=3