Re: [PATCH bpf-next] selftsets/bpf: Retry map update on helper_fill_hashmap()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `can_retry` function incorrectly uses exact equality instead of a bitwise AND to check for `BPF_F_NO_PREALLOC` in `map_opts.map_flags`, preventing retries when other flags are present.
- [Low] The patch modifies a deprecated `CHECK()` macro call without migrating it to modern `ASSERT_*()` macros.
--

commit 39b0e917600bcb89279f4b9e224c7a51b913c4fb
Author: Gabriele Monaco <gmonaco@redhat.com>

selftsets/bpf: Retry map update on helper_fill_hashmap()

This commit enables retrying map updates in helper_fill_hashmap() to handle
transient ENOMEM errors on PREEMPT_RT kernels. It does this by moving the
can_retry() logic up in the file and updating the update loop.

> diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c
> index ccc5acd55ff9d..c32da7bd8be27 100644
> --- a/tools/testing/selftests/bpf/test_maps.c
> +++ b/tools/testing/selftests/bpf/test_maps.c
> @@ -260,6 +260,16 @@ static void test_hashmap_percpu(unsigned int task, void *data)
>  	close(fd);
>  }
>  
> +#define MAP_RETRIES 20
> +
> +static bool can_retry(int err)
> +{
> +	return (err == EAGAIN || err == EBUSY ||
> +		((err == ENOMEM || err == E2BIG) &&
> +		 map_opts.map_flags == BPF_F_NO_PREALLOC));
> +}

[Severity: Medium]
Will this exact equality check prevent retries if other bitmask flags are set?

When helper_fill_hashmap() is called by test_hashmap_zero_seed(),
map_opts.map_flags is explicitly modified to include BPF_F_ZERO_SEED. 

This causes the equality check to evaluate to false even when BPF_F_NO_PREALLOC
is actively set, which bypasses the retry mechanism for this test and
leaves it vulnerable to transient ENOMEM failures on PREEMPT_RT.

Could this be updated to use a bitwise AND instead?

> +
> +
>  #define VALUE_SIZE 3
>  static int helper_fill_hashmap(int max_entries)
>  {
> @@ -274,10 +284,11 @@ static int helper_fill_hashmap(int max_entries)
>  
>  	for (i = 0; i < max_entries; i++) {
>  		key = i; value[0] = key;
> -		ret = bpf_map_update_elem(fd, &key, value, BPF_NOEXIST);
> +		ret = map_update_retriable(fd, &key, value, BPF_NOEXIST,
> +					   MAP_RETRIES, can_retry);
>  		CHECK(ret != 0,
>  		      "can't update hashmap",
> -		      "err: %s\n", strerror(ret));
> +		      "err: %s\n", strerror(-ret));

[Severity: Low]
Since this error checking logic and format arguments are being actively
modified, should this deprecated CHECK() macro be migrated to a modern
ASSERT_OK() macro instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611150704.95133-1-gmonaco@redhat.com?part=1