Re: [PATCH bpf-next v8 2/2] selftests/bpf: Check bpftool batch map dumps

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/bpf: async rhashtable resize race on RHASH dump in run_case()
--

commit 6de2831c6843be67feeb4ee441d5b150a4946b8a
Author: Tianyi Chen <hi@tychen.cc>

selftests/bpf: Check bpftool batch map dumps

This commit adds a BPF selftest that verifies bpftool batched map dumps
produce the same plain and JSON output as individual element lookups across
supported map types, including fallback handling when batch lookups return
-EBUSY.

> diff --git a/tools/testing/selftests/bpf/prog_tests/bpftool_map_batch.c b/tools/testing/selftests/bpf/prog_tests/bpftool_map_batch.c
> new file mode 100644
> index 0000000000000..5cf764bb5e362
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/bpftool_map_batch.c
> @@ -0,0 +1,228 @@

[ ... ]

> +static void run_case(struct bpftool_map_batch *skel, const struct dump_case *test,
> +		     char *expected, char *output)
> +{
> +	LIBBPF_OPTS(bpf_map_create_opts, opts);
> +	int fd = -1, inner_fd = -1, cpus = 1, cpu;

[ ... ]

> +	if (test->type == BPF_MAP_TYPE_LPM_TRIE || test->type == BPF_MAP_TYPE_RHASH)
> +		opts.map_flags = BPF_F_NO_PREALLOC;
> +	max_entries = test->entries ?: 1;
> +	/* Leave room for per-CPU LRU free lists instead of testing eviction. */
> +	if (test->type == BPF_MAP_TYPE_LRU_HASH ||
> +	    test->type == BPF_MAP_TYPE_LRU_PERCPU_HASH)
> +		max_entries *= 4;
> +
> +	fd = bpf_map_create(test->type, "batch_dump", test->key_size,
> +			    test->value_size, max_entries, &opts);

[Severity: Medium]
Can the rhash subtest in test_bpftool_map_batch() race with asynchronous
rhashtable resizing during check_dump()?

In run_case(), opts.map_extra is left at 0 when creating a BPF_MAP_TYPE_RHASH
map with 100 entries. In the kernel, rhtab_map_alloc() passes
params.nelem_hint = (u32)attr->map_extra to rhashtable_init(), so the
rhashtable starts with the default 64 buckets (HASH_DEFAULT_SIZE).

During the 100-element bpf_map_update_elem() loop in run_case(), inserting
the 49th element exceeds the 75% load threshold (rht_grow_above_75()) in
__rhashtable_insert_fast() and schedules rht_deferred_worker() on system_wq
to resize the table from 64 to 128 buckets. Inserting the 97th element then
exceeds the 75% threshold of the 128-bucket table and queues a second
asynchronous resize to 256 buckets just three updates before check_dump()
runs:

run_case()
  bpf_map_update_elem()
    rhtab_map_update_elem()
      rhashtable_lookup_get_insert_key()
        __rhashtable_insert_fast()
          if (rht_grow_above_75(ht, tbl))
            schedule_work(&ht->run_work); -> rht_deferred_worker()

If rht_deferred_worker() runs concurrently with or between the bpftool dumps
in check_dump(), can an in-flight rehash cause bpf_map_get_next_key() to skip
or revisit elements (failing the individual_lookups assertion), or finish
rehashing with a new hash seed and bucket count between the individual dump
and the batch dump so that their iteration order differs (failing
check_same() on batch_output)?

Would setting opts.map_extra = max_entries for BPF_MAP_TYPE_RHASH avoid
scheduling background resizes by pre-sizing the rhashtable to 256 buckets at
creation time?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793818387.git.hi@tychen.cc?part=2