Re: [PATCH 4/4] selftests/bpf: Test rhtab special-field combinations

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Passing an undersized buffer to `bpf_map_update_elem` causes an out-of-bounds read on the userspace stack.
- [Medium] Missing file descriptor validation after manual BPF program lookup.
- [Medium] The timer cancellation test is ineffective because it attempts to cancel a one-shot timer that has already expired.
- [Low] Multi-line comment does not follow the mandatory BPF subsystem style.
--

--- Patch [4]: [PATCH 4/4] selftests/bpf: Test rhtab special-field combinations ---
commit 97a820b266ff0381e757c9ee521aee4aca7caef2
Author: Yuan Chen <[email protected]>

selftests/bpf: Test rhtab special-field combinations

This commit adds selftests for BPF_MAP_TYPE_RHASH special-field combinations,
verifying the recycling of memory for various field types like spin locks,
timers, and kptrs.

> diff --git a/tools/testing/selftests/bpf/prog_tests/rhtab_fields.c b/tools/testing/selftests/bpf/prog_tests/rhtab_fields.c
> new file mode 100644
> index 0000000000000..29de05bcbd4b0
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/rhtab_fields.c

[ ... ]

> @@ -28,34 +28,45 @@ static int run_prog(struct rhtab_fields *skel, const char *name)
>  {
>  	LIBBPF_OPTS(bpf_test_run_opts, topts);
>  	struct bpf_program *prog;
>  	int err;
>  
>  	prog = bpf_object__find_program_by_name(skel->obj, name);
>  	if (!ASSERT_OK_PTR(prog, name))
>  		return -1;
>  	err = bpf_prog_test_run_opts(bpf_program__fd(prog), &topts);

[Severity: Medium]
Is it necessary to validate the file descriptor returned by bpf_program__fd()
before using it? According to BPF selftest patterns, file descriptors from
manual lookups should be verified (>= 0) before use, as they can return a
negative value if the program is invalid.

>  	if (!ASSERT_OK(err, name))
>  		return -1;
>  	return topts.retval;
>  }
>  
>  static void recycle_loop(struct rhtab_fields *skel, int map_fd,
>  			 const char *init, const char *del,
>  			 const char *upd, const char *probe)
>  {
>  	u64 zero = 0;
>  	u32 key = 0;
>  	int i;
>  
>  	for (i = 0; i < RECYCLE_LOOPS; i++) {
>  		if (run_prog(skel, init) != 0) {
>  			/* Element may be gone; recreate and retry once. */
>  			if (!ASSERT_OK(bpf_map_update_elem(map_fd, &key, &zero, BPF_ANY),

[Severity: Medium]
Could this result in an out-of-bounds read on the userspace stack? The zero
variable is 8 bytes, but this function is called for maps like lkmap which
expects a 24-byte value structure. bpf_map_update_elem() will read
map->value_size bytes from this pointer.

>  				       "recreate_elem"))
>  				return;

[ ... ]

> @@ -119,25 +130,24 @@ static void subtest_timer(struct rhtab_fields *skel)
>  {
>  	u64 zero = 0;
>  	u32 key = 0;
>  	int fired, map_fd;
>  
>  	map_fd = bpf_map__fd(skel->maps.tmap);
>  	if (!ASSERT_OK(bpf_map_update_elem(map_fd, &key, &zero, BPF_ANY),

[Severity: Medium]
Could this also lead to an out-of-bounds read? The tmap map value
(struct timer_val) is 24 bytes in size, but it's being updated from the
8-byte zero variable.

>  		       "create_elem"))
>  		return;
>  
>  	if (!ASSERT_OK(run_prog(skel, "arm_timer"), "arm_timer_first"))
>  		return;
>  	usleep(300000);
>  	if (!ASSERT_GT(skel->bss->timer_fired, 0, "timer_fired_first"))
>  		return;
>  
>  	/* Deleting the element must cancel the timer. */
>  	fired = skel->bss->timer_fired;
>  	if (!ASSERT_OK(bpf_map_delete_elem(map_fd, &key), "delete_elem"))
>  		return;
>  	usleep(300000);
>  	ASSERT_EQ(skel->bss->timer_fired, fired, "timer_cancelled_after_delete");

[Severity: Medium]
Does this assertion actually test the cancellation of the timer? Since
arm_timer() sets a 50us one-shot timer and this code sleeps for 300ms
before calling bpf_map_delete_elem(), the timer has already fired by the
time the element is deleted.

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/rhtab_fields.c b/tools/testing/selftests/bpf/progs/rhtab_fields.c
> new file mode 100644
> index 0000000000000..85335f19f1722
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/rhtab_fields.c

[ ... ]

> @@ -89,20 +89,20 @@ struct {
>  	__uint(max_entries, 9);
>  	__type(key, __u32);
>  	__type(value, __u64);
>  } counters SEC(".maps");
>  
> +/* 0: lk init ok, 1: lk probe xchg non-NULL, 2: lk probe xchg NULL,
> + * 3: lk probe magic ok, 4: u init ok, 5: u probe ptr non-NULL,
> + * 6: pc init ok, 7: pc probe xchg non-NULL, 8: pc probe xchg NULL
> + */

[Severity: Low]
This isn't a bug, but does this multi-line comment format violate the
BPF subsystem style? BPF subsystem multi-line comments should have the opening
/* on its own line.

>  static __always_inline void bump(u32 idx)
>  {
>  	u64 *v = bpf_map_lookup_elem(&counters, &idx);
>  
>  	if (v)
>  		(*v)++;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4