Re: [PATCH bpf-next v2 8/8] selftests/bpf: Check bpf_timer value membership

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] selftests/bpf: non-standard multi-line comment style in
  bpf_timer_membership.c
- [Low] selftests/bpf: unnecessary macros and duplication in
  bpf_timer_membership
--

commit 62751ca885fba4775d346fbce832c200968a6cd0
Author: Yun Lu <luyun@kylinos.cn>

selftests/bpf: Check bpf_timer value membership

This commit adds BPF selftests verifying that bpf_timer_init() rejects
deleted or replaced map values with -ENOENT across hash, rhash, and LRU
hash maps while keeping live hash and array timers functional.

> diff --git a/tools/testing/selftests/bpf/progs/bpf_timer_membership.c b/tools/testing/selftests/bpf/progs/bpf_timer_membership.c
> new file mode 100644
> index 0000000000000..b7a10a80f2cf5
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/bpf_timer_membership.c
> @@ -0,0 +1,161 @@

[ ... ]

> +/* Configure and start only a current map member. */
> +#define TIMER_INIT_START(v, map_sym, cb)				\
> +	({								\
> +		long __e;						\
> +		(v)->cookie = cookie;					\
> +		__e = bpf_timer_init(&(v)->timer, &map_sym, CLOCK_MONOTONIC); \
> +		if (!__e) {						\
> +			res_set_err = bpf_timer_set_callback(&(v)->timer, cb); \
> +			if (!res_set_err)				\
> +				res_start_err = bpf_timer_start(&(v)->timer, \
> +								TIMER_NS, 0); \
> +		}							\
> +		__e;							\
> +	})
> +
> +/* Delete and initialize through the retained pointer in one RCU section.
> + * Never configure or start a timer through that pointer, even if init succeeds.
> + */

[Severity: Low]
This isn't a bug, but BPF coding style places the opening /* of a
multi-line comment on its own line with the comment text starting on the
next line.

> +#define TIMER_DELETE_INIT(map_sym, v)					\
> +	({								\
> +		long __e;						\
> +		bpf_rcu_read_lock();					\
> +		res_delete_err = bpf_map_delete_elem(&map_sym, &key);	\
> +		__e = res_delete_err;					\
> +		if (!__e)						\
> +			res_init_err = bpf_timer_init(&(v)->timer, &map_sym, \
> +						      CLOCK_MONOTONIC);	\
> +		bpf_rcu_read_unlock();					\
> +		__e;							\
> +	})
> +
> +#define DEFINE_TIMER_MAP(map_sym, value_type)				\
> +static int map_sym##_cb(void *map, int *key, void *value)		\
> +{									\
> +	value_type *v = value;						\
> +									\
> +	__sync_fetch_and_add(&cb_count[v->cookie & (CB_SLOTS - 1)], 1);	\
> +	return 0;							\
> +}									\
> +									\
> +SEC("syscall")								\
> +int map_sym##_run(void *ctx)						\
> +{									\
> +	value_type *v, *nv, zero = {};					\
> +	int key = 0;							\

[ ... ]

> +struct {
> +	__uint(type, BPF_MAP_TYPE_HASH);
> +	__uint(map_flags, BPF_F_NO_PREALLOC);
> +	__uint(max_entries, 4);
> +	__type(key, int);
> +	__type(value, struct timer_value);
> +} values SEC(".maps");
> +
> +DEFINE_TIMER_MAP(values, struct timer_value)

[Severity: Low]
This isn't a bug, but DEFINE_TIMER_MAP() is only instantiated once for
values, TIMER_DELETE_INIT() is a single-use macro that implicitly captures
key from its caller and returns an unused value, and TIMER_INIT_START()
could be a static __always_inline function.  In addition, cb_count has 64
slots even though every subtest opens a fresh skeleton and passes cookie =
1.

More broadly, prog_tests/bpf_timer_membership.c and
progs/bpf_timer_membership.c duplicate bpf_wq_membership.c from commit
"selftests/bpf: Check bpf_wq value membership" almost verbatim, and the
delete_rhash subtest overlaps with deleted_init_rejected in
prog_tests/rhash_timer.c from commit "bpf: Check bpf_timer value
membership before use".  Replacing the single-use macros with regular
functions and consolidating the shared wq and timer membership test logic
would avoid this duplication.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106095747.471718-1-luyun_611@163.com?part=8