Re: [PATCH bpf-next 04/38] bpf: don't hand out a linked element as extra element

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: l_old stored in extra_elems before check_and_cancel_fields()
--

commit 2a85541dd93d84eaa60a85ebbff00bfad598417f
Author: Josef Bacik <josef@toxicpanda.com>

bpf: don't hand out a linked element as extra element

This commit atomically clears the per-CPU extra_elems slot when claiming a
spare element during preallocated hash map updates, and delays storing the
replaced element into the slot until after it has been unlinked from its
bucket.

> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index 9d4791ee17411..923fec694747b 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
> @@ -1279,11 +1279,17 @@ static struct htab_elem *alloc_htab_elem(struct bpf_htab *htab, void *key,
>  	if (prealloc) {
>  		if (old_elem) {
>  			/* if we're updating the existing element,
> -			 * use per-cpu extra elems to avoid freelist_pop/push
> +			 * use per-cpu extra elems to avoid freelist_pop/push.
> +			 * The caller puts old_elem into the slot once it is
> +			 * unlinked. Until then the slot is empty for an NMI
> +			 * or a kprobe that updates this map on this cpu.
>  			 */
>  			pl_new = this_cpu_ptr(htab->extra_elems);
> -			l_new = *pl_new;
> -			*pl_new = old_elem;
> +			l_new = READ_ONCE(*pl_new);
> +			do {
> +				if (!l_new)
> +					return ERR_PTR(-EBUSY);
> +			} while (!try_cmpxchg_local(pl_new, &l_new, NULL));
>  		} else {
>  			struct pcpu_freelist_node *l;
>  
> @@ -1459,11 +1465,14 @@ static long htab_map_update_elem(struct bpf_map *map, void *key, void *value,
>  	if (l_old) {
>  		hlist_nulls_del_rcu(&l_old->hash_node);
>  
> -		/* l_old has already been stashed in htab->extra_elems, cancel
> -		 * its reusable special fields before it is available for reuse.
> +		/* l_old replaces the extra element that alloc_htab_elem() took,
> +		 * cancel its reusable special fields before the next update on
> +		 * this cpu reuses it.
>  		 */
> -		if (htab_is_prealloc(htab))
> +		if (htab_is_prealloc(htab)) {
> +			WRITE_ONCE(*this_cpu_ptr(htab->extra_elems), l_old);
>  			check_and_cancel_fields(htab, l_old);
> +		}
>  	}
>  	htab_unlock_bucket(b, flags);
>  	if (l_old && !htab_is_prealloc(htab))

[Severity: High]
In kernel/bpf/hashtab.c:htab_map_update_elem(), l_old is written to
htab->extra_elems before check_and_cancel_fields(htab, l_old) is called:

    if (htab_is_prealloc(htab)) {
        WRITE_ONCE(*this_cpu_ptr(htab->extra_elems), l_old);
        check_and_cancel_fields(htab, l_old);
    }

Once WRITE_ONCE(*this_cpu_ptr(htab->extra_elems), l_old) executes, the
per-CPU slot is no longer NULL. If an NMI or a tracing program (for example,
fentry on bpf_obj_cancel_fields()) triggers on the same CPU and updates an
existing key in another bucket of the same preallocated map,
alloc_htab_elem() will claim l_old instead of returning -EBUSY.

When the map contains BTF special fields (BPF_TIMER, BPF_WORKQUEUE, or
BPF_TASK_WORK), the nested update can link l_old into the other bucket while
its previous timer, workqueue, or task_work fields are still active, and then
the outer check_and_cancel_fields(htab, l_old) will cancel and free the
special fields of the newly inserted live element.

Should check_and_cancel_fields(htab, l_old) be called before
WRITE_ONCE(*this_cpu_ptr(htab->extra_elems), l_old) so the slot stays NULL
until the special fields have been canceled?

титься

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017012058.355787-1-alexei.starovoitov@gmail.com?part=4