Skip to content

Commit

Permalink
bpf: skip non existing key in generic_map_lookup_batch
Browse files Browse the repository at this point in the history 
The generic_map_lookup_batch currently returns EINTR if it fails with
ENOENT and retries several times on bpf_map_copy_value. The next batch
would start from the same location, presuming it's a transient issue.
This is incorrect if a map can actually have "holes", i.e.
"get_next_key" can return a key that does not point to a valid value. At
least the array of maps type may contain such holes legitly. Right now
these holes show up, generic batch lookup cannot proceed any more. It
will always fail with EINTR errors.

Rather, do not retry in generic_map_lookup_batch. If it finds a non
existing element, skip to the next key.

Fixes: cb4d03a ("bpf: Add generic support for lookup batch op")
Closes: https://lore.kernel.org/bpf/[email protected]/
Signed-off-by: Yan Zhai <[email protected]>
  • Loading branch information
Yan Zhai authored and Kernel Patches Daemon committed Feb 5, 2025
1 parent 928f200 commit 69364f2
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 25 deletions.
16 changes: 5 additions & 11 deletions kernel/bpf/syscall.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1988,7 +1988,7 @@ int generic_map_lookup_batch(struct bpf_map *map,
void __user *values = u64_to_user_ptr(attr->batch.values);
void __user *keys = u64_to_user_ptr(attr->batch.keys);
void *buf, *buf_prevkey, *prev_key, *key, *value;
int err, retry = MAP_LOOKUP_RETRIES;
int err;
u32 value_size, cp, max_count;

if (attr->batch.elem_flags & ~BPF_F_LOCK)
Expand Down Expand Up @@ -2035,14 +2035,8 @@ int generic_map_lookup_batch(struct bpf_map *map,
err = bpf_map_copy_value(map, key, value,
attr->batch.elem_flags);

if (err == -ENOENT) {
if (retry) {
retry--;
continue;
}
err = -EINTR;
break;
}
if (err == -ENOENT)
goto next_key;

if (err)
goto free_buf;
Expand All @@ -2057,12 +2051,12 @@ int generic_map_lookup_batch(struct bpf_map *map,
goto free_buf;
}

cp++;
next_key:
if (!prev_key)
prev_key = buf_prevkey;

swap(prev_key, key);
retry = MAP_LOOKUP_RETRIES;
cp++;
cond_resched();
}

Expand Down
54 changes: 40 additions & 14 deletions tools/testing/selftests/bpf/map_tests/map_in_map_batch_ops.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,11 +120,12 @@ static void validate_fetch_results(int outer_map_fd,

static void fetch_and_validate(int outer_map_fd,
struct bpf_map_batch_opts *opts,
__u32 batch_size, bool delete_entries)
__u32 batch_size, bool delete_entries,
bool has_holes)
{
int err, max_entries = OUTER_MAP_ENTRIES - !!has_holes;
__u32 *fetched_keys, *fetched_values, total_fetched = 0;
__u32 batch_key = 0, fetch_count, step_size;
int err, max_entries = OUTER_MAP_ENTRIES;
__u32 value_size = sizeof(__u32);

/* Total entries needs to be fetched */
Expand All @@ -135,9 +136,9 @@ static void fetch_and_validate(int outer_map_fd,
"error=%s\n", strerror(errno));

for (step_size = batch_size;
step_size <= max_entries;
step_size < max_entries + batch_size; /* allow read partial */
step_size += batch_size) {
fetch_count = step_size;
fetch_count = batch_size;
err = delete_entries
? bpf_map_lookup_and_delete_batch(outer_map_fd,
total_fetched ? &batch_key : NULL,
Expand Down Expand Up @@ -184,18 +185,19 @@ static void fetch_and_validate(int outer_map_fd,
}

static void _map_in_map_batch_ops(enum bpf_map_type outer_map_type,
enum bpf_map_type inner_map_type)
enum bpf_map_type inner_map_type,
bool has_holes)
{
__u32 max_entries = OUTER_MAP_ENTRIES - !!has_holes;
__u32 *outer_map_keys, *inner_map_fds;
__u32 max_entries = OUTER_MAP_ENTRIES;
LIBBPF_OPTS(bpf_map_batch_opts, opts);
__u32 value_size = sizeof(__u32);
int batch_size[2] = {5, 10};
__u32 map_index, op_index;
int outer_map_fd, ret;

outer_map_keys = calloc(max_entries, value_size);
inner_map_fds = calloc(max_entries, value_size);
outer_map_keys = calloc(OUTER_MAP_ENTRIES, value_size);
inner_map_fds = calloc(OUTER_MAP_ENTRIES, value_size);
CHECK((!outer_map_keys || !inner_map_fds),
"Memory allocation failed for outer_map_keys or inner_map_fds",
"error=%s\n", strerror(errno));
Expand All @@ -209,6 +211,24 @@ static void _map_in_map_batch_ops(enum bpf_map_type outer_map_type,
((outer_map_type == BPF_MAP_TYPE_ARRAY_OF_MAPS)
? 9 : 1000) - map_index;

/* This condition is only meaningful for array of maps.
*
* max_entries == OUTER_MAP_ENTRIES - 1 if it is true. Say
* max_entries is short for n, then outer_map_keys looks like:
*
* [n, n-1, ... 2, 1]
*
* We change it to
*
* [n, n-1, ... 2, 0]
*
* So it will leave key 1 as a hole. It will serve to test the
* correctness when batch on an array: a "non-exist" key might be
* actually allocated and returned from key iteration.
*/
if (has_holes)
outer_map_keys[max_entries - 1]--;

/* batch operation - map_update */
ret = bpf_map_update_batch(outer_map_fd, outer_map_keys,
inner_map_fds, &max_entries, &opts);
Expand All @@ -219,12 +239,14 @@ static void _map_in_map_batch_ops(enum bpf_map_type outer_map_type,
/* batch operation - map_lookup */
for (op_index = 0; op_index < 2; ++op_index)
fetch_and_validate(outer_map_fd, &opts,
batch_size[op_index], false);
batch_size[op_index], false,
has_holes);

/* batch operation - map_lookup_delete */
if (outer_map_type == BPF_MAP_TYPE_HASH_OF_MAPS)
fetch_and_validate(outer_map_fd, &opts,
max_entries, true /*delete*/);
max_entries, true /*delete*/,
has_holes);

/* close all map fds */
for (map_index = 0; map_index < max_entries; map_index++)
Expand All @@ -237,16 +259,20 @@ static void _map_in_map_batch_ops(enum bpf_map_type outer_map_type,

void test_map_in_map_batch_ops_array(void)
{
_map_in_map_batch_ops(BPF_MAP_TYPE_ARRAY_OF_MAPS, BPF_MAP_TYPE_ARRAY);
_map_in_map_batch_ops(BPF_MAP_TYPE_ARRAY_OF_MAPS, BPF_MAP_TYPE_ARRAY, false);
printf("%s:PASS with inner ARRAY map\n", __func__);
_map_in_map_batch_ops(BPF_MAP_TYPE_ARRAY_OF_MAPS, BPF_MAP_TYPE_HASH);
_map_in_map_batch_ops(BPF_MAP_TYPE_ARRAY_OF_MAPS, BPF_MAP_TYPE_HASH, false);
printf("%s:PASS with inner HASH map\n", __func__);
_map_in_map_batch_ops(BPF_MAP_TYPE_ARRAY_OF_MAPS, BPF_MAP_TYPE_ARRAY, true);
printf("%s:PASS with inner ARRAY map with holes\n", __func__);
_map_in_map_batch_ops(BPF_MAP_TYPE_ARRAY_OF_MAPS, BPF_MAP_TYPE_HASH, true);
printf("%s:PASS with inner HASH map with holes\n", __func__);
}

void test_map_in_map_batch_ops_hash(void)
{
_map_in_map_batch_ops(BPF_MAP_TYPE_HASH_OF_MAPS, BPF_MAP_TYPE_ARRAY);
_map_in_map_batch_ops(BPF_MAP_TYPE_HASH_OF_MAPS, BPF_MAP_TYPE_ARRAY, false);
printf("%s:PASS with inner ARRAY map\n", __func__);
_map_in_map_batch_ops(BPF_MAP_TYPE_HASH_OF_MAPS, BPF_MAP_TYPE_HASH);
_map_in_map_batch_ops(BPF_MAP_TYPE_HASH_OF_MAPS, BPF_MAP_TYPE_HASH, false);
printf("%s:PASS with inner HASH map\n", __func__);
}

0 comments on commit 69364f2

Please sign in to comment.