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] [thread-next>] [day] [month] [year] [list]

Message-ID: <5b801c37-d9e8-48f8-a0bc-4acd03a2acd6@linux.dev>
Date: Thu, 8 Jan 2026 12:40:29 -0800
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: ameryhung@...il.com
Cc: bot+bpf-ci@...nel.org, bpf@...r.kernel.org, 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,
 kernel-team@...a.com, ast@...nel.org, eddyz87@...il.com, clm@...a.com,
 ihor.solodrai@...ux.dev
Subject: Re: [PATCH bpf-next v3 01/16] bpf: Convert bpf_selem_unlink_map to
 failable


On 12/18/25 10:27 AM, bot+bpf-ci@...nel.org wrote:
>> 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.

I just catch up on this ai-review, so I made a similar comment on this 
raw_spinlock in another reply instead of here. imo, we should still 
avoid this even the rqspinlock is used in the later patch.