Re: [PATCH 2/3] selftests/hid: Add a test to ensure we can write fields in hid_device
flat view
From: sashiko-bot@kernel.org
Date: 2026-08-25 08:58:24
Also in:
bpf
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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)
+{[ ... ]
quoted hunk ↗ jump to hunk
+ 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. [ ... ]
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
+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