netdev - Re: [PATCH net-next] af_unix: Fix garbage collector racing against send() MSG

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: <a8cfc1ce-4ab5-4738-846c-73084b621b9f@rbox.co>
Date: Tue, 7 May 2024 21:05:12 +0200
From: Michal Luczaj <mhal@...x.co>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
 netdev@...r.kernel.org, pabeni@...hat.com
Subject: Re: [PATCH net-next] af_unix: Fix garbage collector racing against
 send() MSG_OOB

On 5/7/24 19:26, Kuniyuki Iwashima wrote:
> From: Michal Luczaj <mhal@...x.co>
> Date: Tue,  7 May 2024 18:29:33 +0200>>
>> ...
>> diff --git a/net/unix/garbage.c b/net/unix/garbage.c
>> index d76450133e4f..f2098653aef8 100644
>> --- a/net/unix/garbage.c
>> +++ b/net/unix/garbage.c
>> @@ -357,11 +357,10 @@ static void unix_collect_skb(struct list_head *scc, struct sk_buff_head *hitlist
>>  		u = edge->predecessor;
>>  		queue = &u->sk.sk_receive_queue;
>>  
>> -		spin_lock(&queue->lock);
>> -
>>  		if (u->sk.sk_state == TCP_LISTEN) {
>>  			struct sk_buff *skb;
>>  
>> +			spin_lock(&queue->lock);
>>  			skb_queue_walk(queue, skb) {
>>  				struct sk_buff_head *embryo_queue = &skb->sk->sk_receive_queue;
>>  
>> @@ -370,18 +369,21 @@ static void unix_collect_skb(struct list_head *scc, struct sk_buff_head *hitlist
>>  				skb_queue_splice_init(embryo_queue, hitlist);
>>  				spin_unlock(&embryo_queue->lock);
>>  			}
>> +			spin_unlock(&queue->lock);
>>  		} else {
>> +			spin_lock(&queue->lock);
>>  			skb_queue_splice_init(queue, hitlist);
>> +			spin_unlock(&queue->lock);
>>  
>>  #if IS_ENABLED(CONFIG_AF_UNIX_OOB)
>> +			unix_state_lock_nested(&u->sk, U_LOCK_GC_OOB);
> 
> I've just noticed you posted a similar patch to the same issue :)
> https://lore.kernel.org/netdev/20240507170018.83385-1-kuniyu@amazon.com/

Yeah, I'm really sorry for the noise. I'm not on netdev and haven't seen
your patch :(

> But we cannot acquire unix_state_lock() here for receiver sockets
> (listener is ok) because unix_(add|remove|update)_edges() takes
> unix_state_lock() first and then unix_gc_lock.
> 
> So, we need to avoid the race by updating oob_skb under recvq lock
> in queue_oob().

Arrgh, sure, now I get it. Thanks!

Michal