Re: [PATCH bpf-next v9 2/2] bpf: htab: Reduce elem_size by 8 bytes for small key sizes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: lockless RCU reader race due to delayed key overwrite in recycled elements
--

commit 4208dd21bf1b230b07d30630716cc32b6b4f2404
Author: T.J. Mercier <tjmercier@google.com>

bpf: htab: Reduce elem_size by 8 bytes for small key sizes

This commit reduces the element size for standard and PCPU hash maps
with small key sizes by removing the 32-bit hash value. For these cases,
only the key field guards lookups, and the new key assignment is moved
to the end of alloc_htab_elem().

> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index 810db9c436534..9d4791ee17411 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c

[ ... ]

> @@ -1241,7 +1347,11 @@ static struct htab_elem *alloc_htab_elem(struct bpf_htab *htab, void *key,
>  		copy_map_value(&htab->map, htab_elem_value(l_new, key_size), value);
>  	}
>  
> -	l_new->hash = hash;
> +	if (htab_has_hash(htab))
> +		htab_elem_set_hash(l_new, hash);
> +	else
> +		WRITE_ONCE(*(unsigned long *)l_new->key,
> +			   htab_zero_extend_key(key, key_size));

[Severity: High]
Can this delayed key overwrite lead to a race with lockless RCU readers
when elements are recycled?

When a preallocated element is recycled, it retains its old key until this
WRITE_ONCE. However, copy_map_value() was already called just above this
location to populate the value for the new key.

If a lockless reader is concurrently executing __lookup_elem_raw() and
searching for the old key, the reader only checks the key without a hash:

    if (READ_ONCE(*(unsigned long *)l->key) == k)

Since the old key is left intact while the value is updated, couldn't the
reader successfully match the old key but erroneously return the new,
unrelated value?

Would it be safer to invalidate the key before calling copy_map_value(),
perhaps combined with a memory barrier, to prevent a reader from matching
the old key while its value is being overwritten?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008104108.993791-1-tjmercier@google.com?part=2