Re: [PATCH bpf-next v7 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: RCU readers can match recycled elements causing UAF/corruption
--

commit 1b9a82a1c5a80781d8a76b226efc1db28e064ff2
Author: T.J. Mercier <tjmercier@google.com>

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

This commit reduces element size for standard and non-LRU per-CPU hash
maps with small keys by omitting the 32-bit hash value. For these small
keys, it relies on a single atomic instruction for key comparison and moves
the new key assignment to the end of element allocation to avoid exposing
uninitialized values.

> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index 5db11a21ce980..7a8049cc99828 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c

[ ... ]

> @@ -734,35 +784,60 @@ static inline struct hlist_nulls_head *select_bucket(struct bpf_htab *htab, u32
>  	return &__select_bucket(htab, hash)->head;
>  }
>  
> -/* this lookup function can only be called with bucket lock taken */
> -static struct htab_elem *lookup_elem_raw(struct hlist_nulls_head *head, u32 hash,
> -					 void *key, u32 key_size)
> +static __always_inline struct htab_elem *
> +__lookup_elem_raw(struct bpf_htab *htab, struct hlist_nulls_head *head,
> +		  u32 hash, void *key, u32 key_size,
> +		  struct hlist_nulls_node **out_n)
>  {
>  	struct hlist_nulls_node *n;
>  	struct htab_elem *l;
>  
> -	hlist_nulls_for_each_entry_rcu(l, n, head, hash_node)
> -		if (l->hash == hash && !memcmp(&l->key, key, key_size))
> -			return l;
> +	if (htab_has_hash(htab)) {
> +		hlist_nulls_for_each_entry_rcu(l, n, head, hash_node)
> +			if (htab_elem_hash(l) == hash &&
> +			    !memcmp(&l->key, key, key_size))
> +				return l;
> +	} else {
> +		/*
> +		 * When hash is omitted, key comparisons must be atomic. Zero extend
> +		 * the caller's key to the word size to support an atomic compare.
> +		 */
> +		unsigned long k = 0;
> +
> +		memcpy(&k, key, key_size);
> +		hlist_nulls_for_each_entry_rcu(l, n, head, hash_node)
> +			if (READ_ONCE(*(unsigned long *)l->key) == k)
> +				return l;
> +	}

[Severity: High]
Can an RCU reader searching for a deleted key incorrectly match a recycled
element here?

Without a hash field to act as an epoch check, it seems a lockless reader
might accept an element that is currently being reallocated if the stale
key hasn't been overwritten yet.

[ ... ]

> @@ -1225,7 +1302,18 @@ 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);
>  	}

[Severity: High]
If a lockless reader successfully matched the old key in __lookup_elem_raw(),
could it observe the element in an inconsistent state here?

While the value is being copied (or a new per-CPU pointer is being initialized
earlier in this function), the reader might read corrupted data. In the case
of a non-preallocated per-CPU map, could it dereference the old, freed pptr
leading to a use-after-free?

> -	l_new->hash = hash;
> +	if (htab_has_hash(htab)) {
> +		htab_elem_set_hash(l_new, hash);
> +	} else {
> +		/*
> +		 * Zero-extend key into k for an atomic write to support
> +		 * lockless RCU readers.
> +		 */
> +		unsigned long k = 0;
> +
> +		memcpy(&k, key, key_size);
> +		WRITE_ONCE(*(unsigned long *)l_new->key, k);
> +	}
>  	return l_new;

[Severity: High]
Does delaying the key update to the end of alloc_htab_elem() inadvertently
leave the old key intact while the new value is initialized?

While this prevents premature matches on the new key, it seems to expose
the element to readers searching for the old key, breaking the RCU safety
of recycled elements.

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