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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Eliding the 32-bit hash field for small keys causes lockless RCU lookups to suffer from torn reads, leading to false positive matches when elements are concurrently recycled.
--

--- Patch [2]: [PATCH bpf-next v2 2/2] bpf: htab: Reduce elem_size by 8 bytes for small key sizes ---
commit 1a9b131810714c16f6fb58fbc067753c917cf3cd
Author: T.J. Mercier <[email protected]>

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

This commit reduces the element size of standard and PCPU hash maps for small
key sizes by removing the 32-bit hash value and padding. It introduces
htab_has_hash() to conditionally bypass hash checking and storage when the key
size is less than or equal to the word size.

> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index f54366da459f5..82b19bd543109 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
[ ... ]
> @@ -618,6 +625,12 @@ static struct bpf_map *htab_map_alloc(union bpf_attr *attr)
>  
>  	bpf_map_init_from_attr(&htab->map, attr);
>  
> +	/* Avoid hash memory use and comparisons where unnecessary.
> +	 * u32 hash reads are always atomic. If we elide them, key comparisons must also be atomic
> +	 * to avoid false positive key matches due to torn key reads / writes. So check key_size.
> +	 */
> +	htab->has_hash = htab_is_lru(htab) || htab->map.key_size > sizeof(unsigned long);

[Severity: High]
While the comment correctly notes that key comparisons must be atomic when the
hash is elided, are runtime memcmp() and memcpy() operations actually atomic
for all key sizes <= 8 bytes (such as 3, 5, 6, or 7 bytes)?

[ ... ]
> @@ -761,10 +776,16 @@ static struct htab_elem *lookup_elem_raw(struct bpf_htab *htab,
>  	struct hlist_nulls_node *n;
>  	struct htab_elem *l;
>  
> -	hlist_nulls_for_each_entry_rcu(l, n, head, node.hash_node)
> -		if (htab_elem_hash(htab, l) == hash &&
> -		    !memcmp(htab_elem_key(htab, l), key, key_size))
> -			return l;
> +	if (htab_has_hash(htab)) {
> +		hlist_nulls_for_each_entry_rcu(l, n, head, node.hash_node)
> +			if (htab_elem_hash(htab, l) == hash &&
> +			    !memcmp(htab_elem_key(htab, l), key, key_size))
> +				return l;
> +	} else {
> +		hlist_nulls_for_each_entry_rcu(l, n, head, node.hash_node)
> +			if (!memcmp(htab_elem_key(htab, l), key, key_size))
> +				return l;
> +	}
>  
>  	return NULL;
>  }

[Severity: High]
Can this lockless memcmp() result in a torn read and a false positive match?

In preallocated BPF hash maps, deleted elements are immediately pushed to a
lockless freelist without an RCU grace period. If a reader is preempted while
examining an element, and another thread reallocates and writes to that
element's key using a non-atomic memcpy() (such as in alloc_htab_elem()), the
reader might observe a torn mixture of the old and new key.

Without the 32-bit atomic hash check acting as a filter, could this torn read
randomly match the reader's target key, causing the lookup to return an
unrelated map element?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2