Re: [PATCH 2/3] selftests/hid: Add a test to ensure we can write fields in hid_device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Flaky test due to off-by-one error when searching for the null terminator of a device ID string.
- [Low] Incorrect macro undefinition leaves `HID_BPF_MEMCPY` defined and attempts to undefine a nonexistent macro.
--

--- Patch [2]: [PATCH 2/3] selftests/hid: Add a test to ensure we can write fields in hid_device ---
commit ac5a75219b6789481c51176040f60dd282ac248f
Author: Benjamin Tissoires <bentiss@kernel.org>

selftests/hid: Add a test to ensure we can write fields in hid_device

This commit adds a selftest to verify that the name, uniq, and phys
fields of struct hid_device can be modified by a BPF program. This
ensures these fields remain writeable for BPF programs.

> diff --git a/tools/testing/selftests/hid/hid_bpf.c b/tools/testing/selftests/hid/hid_bpf.c
> index b851339308c21..069ebdbb4d1c4 100644
> --- a/tools/testing/selftests/hid/hid_bpf.c
> +++ b/tools/testing/selftests/hid/hid_bpf.c
[ ... ]
> +TEST_F(hid_bpf, test_rdesc_fixup_change_uniq_name_phys)
> +{
[ ... ]
> +	snprintf(expected, sizeof(expected), "%d phys:coming:from:bpf", self->hid.dev_id);
> +
> +	err = ioctl(self->hidraw_fd, HIDIOCGRAWPHYS(sizeof(buf)), buf);
> +	ASSERT_GE(err, 0) TH_LOG("HIDIOCGRAWPHYS");
> +	ASSERT_STREQ(expected, buf);

[Severity: Medium]
Will this test intermittently trigger a regression if dev_id is 1000 or
greater?

In setup_uhid(), dev_id is set as a random number between 0 and 1023. If
the number is 4 digits long, the string representation takes 4 characters,
placing the null terminator at index 4, which the BPF loop below doesn't
reach. This would cause the ASSERT_STREQ check to fail because the suffix
is not appended.

[ ... ]
> diff --git a/tools/testing/selftests/hid/progs/hid.c b/tools/testing/selftests/hid/progs/hid.c
> index b21fbb13c926f..b5d9aea1bda1d 100644
> --- a/tools/testing/selftests/hid/progs/hid.c
> +++ b/tools/testing/selftests/hid/progs/hid.c
[ ... ]
> +SEC("?struct_ops.s/hid_rdesc_fixup")
> +int BPF_PROG(hid_rdesc_fixup_change_uniq_name_phys, struct hid_bpf_ctx *hid_ctx)
> +{
> +#define HID_BPF_MEMCPY(target, str) \
> +	__builtin_memcpy(target, str, sizeof(str))
> +
> +	HID_BPF_MEMCPY(hid_ctx->hid->name, "name coming from bpf");
> +	HID_BPF_MEMCPY(hid_ctx->hid->uniq, "uniq:coming:from:bpf");
> +	/* hid_bpf relies on a phys being a rand % 1024 */
> +	for (int i = 0; i < 4; i++) {
> +		if (!hid_ctx->hid->phys[i]) {
> +			HID_BPF_MEMCPY(hid_ctx->hid->phys + i, " phys:coming:from:bpf");
> +			break;
> +		}
> +	}

[Severity: Medium]
Could this loop condition cause regressions when the phys string is 4
characters long?

If the random dev_id is >= 1000, the string will take up indices 0
through 3, and the null terminator will be at index 4. The loop stops at
i = 3, so it will miss the null terminator and skip the memcpy. Should the
loop condition be i < 5?

> +
> +#undef HID_BPF_CONCAT

[Severity: Low]
This isn't a bug, but it looks like there is a typo here. Should this be
undefining HID_BPF_MEMCPY instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825-wip-bpf-safe-v1-0-ab9e0e111c5c@kernel.org?part=2