Re: [RFC PATCH bpf-next 5/6] selftests/bpf: Exercise word-at-a-time string kfuncs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Tests intended to exercise word-load page faults fail to reach the word-at-a-time code paths due to unaligned arguments or byte-load implementations.
--

--- Patch [5]: [RFC PATCH bpf-next 5/6] selftests/bpf: Exercise word-at-a-time string kfuncs ---
commit dc0a0a05e577e233a4ad33ac1ebb0ed2f4f840c1
Author: Leon Hwang <leon.hwang@linux.dev>

selftests/bpf: Exercise word-at-a-time string kfuncs
    
This commit adds functional coverage for aligned and unaligned strings
across the scan, comparison, span, and substring families. It also adds tests
using aligned invalid kernel addresses to exercise failed word-sized nofault
loads and their byte-sized retries.

> diff --git a/tools/testing/selftests/bpf/progs/string_kfuncs_failure1.c b/tools/testing/selftests/bpf/progs/string_kfuncs_failure1.c
> index bddc4e8579d24..05f93c7cd07cd 100644
> --- a/tools/testing/selftests/bpf/progs/string_kfuncs_failure1.c
> +++ b/tools/testing/selftests/bpf/progs/string_kfuncs_failure1.c

[ ... ]

> @@ -108,4 +109,61 @@ SEC("syscall") __retval(-EFAULT) int test_strnstr_pagefault2(void *ctx) { return
>  SEC("syscall") __retval(-EFAULT) int test_strncasestr_pagefault1(void *ctx) { return bpf_strncasestr(invalid_kern_ptr, "hello", 1); }
>  SEC("syscall") __retval(-EFAULT) int test_strncasestr_pagefault2(void *ctx) { return bpf_strncasestr("hello", invalid_kern_ptr, 1); }
>  
> +/* Exercise word-load faults and the byte retry at the same address. */
> +SEC("syscall")
> +__retval(-EFAULT)
> +int test_strncasecmp_word_pagefault(void *ctx)
> +{
> +	return bpf_strncasecmp(invalid_aligned_kern_ptr, "12345678", 8);

[Severity: Medium]
Will this actually exercise the word-load fault logic?

Since "12345678" is a string literal, it may not be aligned. If it is
relatively misaligned, __bpf_strncasecmp falls back to a byte-by-byte loop,
which would mean this test misses the intended word-load fault test entirely
and only tests the byte-load path.

> +}

[ ... ]

> +SEC("syscall")
> +__retval(-EFAULT)
> +int test_strspn_set_word_pagefault(void *ctx)
> +{
> +	return bpf_strspn("a", invalid_aligned_kern_ptr);

[Severity: Medium]
Does this test reach the word-at-a-time code path?

It looks like bpf_strspn processes the 'accept' set using strictly 1-byte
loads. By passing the invalid pointer as the set, the test will fault on
a byte-load, bypassing the word-load logic it is intended to test.

> +}

[ ... ]

> +SEC("syscall")
> +__retval(-EFAULT)
> +int test_strnstr_word_pagefault2(void *ctx)
> +{
> +	return bpf_strnstr("12345678", invalid_aligned_kern_ptr, 8);

[Severity: Medium]
Will this fault on a word-load?

Looking at bpf_strnstr(), it unconditionally reads the first character of the
second argument using a 1-byte load. If invalid_aligned_kern_ptr is passed
here, it will fault on the initial byte-load before any word-load occurs.

> +}
> +
>  char _license[] SEC("license") = "GPL";

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728145727.45153-1-leon.hwang@linux.dev?part=5