Re: [PATCH bpf-next v6 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 2 potential issue(s) to consider:
- [High] bpf: htab: Data corruption when RCU readers match old key on recycled elements
- [High] bpf: htab: Missing memory barrier allows lockless readers to see uninitialized values
--

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

bpf: htab: Reduce elem_size by 8 bytes for small key sizes
    
This commit removes the stored hash value from hash table elements when the
key size is small, falling back to direct atomic key comparisons. It also
updates element recycling by reordering new key assignment to occur after
the value has been copied, attempting to prevent early lockless matches.

> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
> @@ -775,9 +777,32 @@ static inline struct hlist_nulls_head *select_bucket(struct bpf_htab *htab, u32
[ ... ]
> +	} 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]
Does this lockless reader lack an acquire memory barrier?

When evaluating the key using READ_ONCE, an acquire barrier such as
smp_load_acquire or smp_rmb before reading the value seems to be missing.
Without this barrier, a concurrent reader on a weakly ordered architecture
might reorder the value read before the key check, observing uninitialized
or stale values.

> +	}
>  
> +	if (out_n)
> +		*out_n = n;
>  	return NULL;
>  }
[ ... ]
> @@ -1268,7 +1268,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]
Can this lead to data corruption when rcu readers match the old key?

When a map element is recycled, the assignment of the new key is delayed to
the end of alloc_htab_elem. This means the old key remains in place while
copy_map_value overwrites the element's value. 

If a concurrent lockless rcu reader searches for the old, deleted key, it
will successfully match the old key during this window and incorrectly read
the newly written value. Because bpf keys are arbitrary bytes, there is no
safe poison value that can be temporarily written to prevent legitimate
concurrent lookups from matching.

>  	}
>  
> -	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);

[Severity: High]
Are memory barriers required when publishing the new hash or key?

Using WRITE_ONCE or htab_elem_set_hash to publish the new key and hash only
prevents compiler reordering. Without a CPU memory barrier like
smp_store_release or smp_wmb, weakly ordered architectures like arm64 can
reorder the key or hash store to occur before the stores in copy_map_value.

This would allow a concurrent rcu reader to observe the newly written key
early and access the value before it has been completely initialized.

> +	}
>  	return l_new;

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