netdev - Re: [PATCH bpf-next v3 01/16] bpf: Convert bpf_selem_unlink

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <1a31d5b805f81b28ea95cdfdd57b8e2fb88ab29bfbbc034b4443978b4dddb5c6@mail.kernel.org>
Date: Thu, 18 Dec 2025 18:27:56 +0000 (UTC)
From: bot+bpf-ci@...nel.org
To: ameryhung@...il.com,bpf@...r.kernel.org
Cc: netdev@...r.kernel.org,alexei.starovoitov@...il.com,andrii@...nel.org,daniel@...earbox.net,memxor@...il.com,martin.lau@...nel.org,kpsingh@...nel.org,yonghong.song@...ux.dev,song@...nel.org,haoluo@...gle.com,ameryhung@...il.com,kernel-team@...a.com,ast@...nel.org,andrii@...nel.org,daniel@...earbox.net,martin.lau@...nel.org,eddyz87@...il.com,yonghong.song@...ux.dev,clm@...a.com,ihor.solodrai@...ux.dev
Subject: Re: [PATCH bpf-next v3 01/16] bpf: Convert bpf_selem_unlink_map to failable

> diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
> index e2fe6c32822b..4e3f227fd634 100644
> --- a/kernel/bpf/bpf_local_storage.c
> +++ b/kernel/bpf/bpf_local_storage.c

[ ... ]

> @@ -488,9 +516,10 @@ bpf_local_storage_update(void *owner, struct bpf_local_storage_map *smap,
>  {
>  	struct bpf_local_storage_data *old_sdata = NULL;
>  	struct bpf_local_storage_elem *alloc_selem, *selem = NULL;
> +	struct bpf_local_storage_map_bucket *b, *old_b = NULL;
> +	unsigned long flags, b_flags, old_b_flags;
>  	struct bpf_local_storage *local_storage;
>  	HLIST_HEAD(old_selem_free_list);
> -	unsigned long flags;
>  	int err;

[ ... ]

> @@ -574,20 +603,37 @@ bpf_local_storage_update(void *owner, struct bpf_local_storage_map *smap,
>  		goto unlock;
>  	}
>
> +	b = select_bucket(smap, selem);
> +
> +	if (old_sdata) {
> +		old_b = select_bucket(smap, SELEM(old_sdata));
> +		old_b = old_b == b ? NULL : old_b;
> +	}
> +
> +	raw_spin_lock_irqsave(&b->lock, b_flags);
> +
> +	if (old_b)
> +		raw_spin_lock_irqsave(&old_b->lock, old_b_flags);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Could this introduce an ABBA deadlock between two bucket locks? If two
threads concurrently update storage on the same map with different owners,
their new and old selems could hash to different buckets with reversed
order:

  Thread 1: new_selem -> bucket_A, old_selem -> bucket_B
            acquires b->lock (A), then waits for old_b->lock (B)

  Thread 2: new_selem -> bucket_B, old_selem -> bucket_A
            acquires b->lock (B), then waits for old_b->lock (A)

Since the locks are regular raw_spinlocks (not rqspinlocks yet), they
will block indefinitely in this scenario.

Note: I see that commit 615477732ee8 ("bpf: Change local_storage->lock
and b->lock to rqspinlock") later in this series changes to rqspinlock
which has deadlock detection and can return -EDEADLK instead of blocking.
However, in the current commit the locks are still raw_spinlocks.

> +
>  	alloc_selem = NULL;
>  	/* First, link the new selem to the map */
> -	bpf_selem_link_map(smap, selem);
> +	bpf_selem_link_map_nolock(smap, selem, b);
>
>  	/* Second, link (and publish) the new selem to local_storage */
>  	bpf_selem_link_storage_nolock(local_storage, selem);
>
>  	/* Third, remove old selem, SELEM(old_sdata) */
>  	if (old_sdata) {
> -		bpf_selem_unlink_map(SELEM(old_sdata));
> +		bpf_selem_unlink_map_nolock(SELEM(old_sdata));
>  		bpf_selem_unlink_storage_nolock(local_storage, SELEM(old_sdata),
>  						&old_selem_free_list);
>  	}
>
> +	if (old_b)
> +		raw_spin_unlock_irqrestore(&old_b->lock, old_b_flags);
> +
> +	raw_spin_unlock_irqrestore(&b->lock, b_flags);
> +
>  unlock:
>  	raw_spin_unlock_irqrestore(&local_storage->lock, flags);


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/20346662777