netdev - Re: [PATCH net] af_unix: Disable MSG_OOB handling for sockets in sockmap/sockhash

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: <fda335cd-3fb1-4024-bff7-aedeb1d8710a@rbox.co>
Date: Sun, 23 Jun 2024 00:38:22 +0200
From: Michal Luczaj <mhal@...x.co>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: bpf@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
 jakub@...udflare.com, john.fastabend@...il.com, kuba@...nel.org,
 netdev@...r.kernel.org, pabeni@...hat.com
Subject: Re: [PATCH net] af_unix: Disable MSG_OOB handling for sockets in
 sockmap/sockhash

On 6/21/24 00:12, Kuniyuki Iwashima wrote:
> Sorry for not mentioning this before, but could you replace "net" with
> "bpf" in Subject and rebase the patch on bpf.git so that we can trigger
> the patchwork's CI ?

No problem, will do.

>> ...
>>  static int unix_stream_read_skb(struct sock *sk, skb_read_actor_t recv_actor)
>>  {
>> +	struct unix_sock *u = unix_sk(sk);
>> +	struct sk_buff *skb;
>> +	int err;
>> +
>>  	if (unlikely(READ_ONCE(sk->sk_state) != TCP_ESTABLISHED))
>>  		return -ENOTCONN;
>>  
>> -	return unix_read_skb(sk, recv_actor);
>> +	mutex_lock(&u->iolock);
>> +	skb = skb_recv_datagram(sk, MSG_DONTWAIT, &err);
> 
> 	mutex_unlock(&u->iolock);
> 
> I think we can drop mutex here as the skb is already unlinked
> and no receiver can touch it.

I guess you're right about the mutex. That said, double mea culpa, lack of
state lock makes things racy:

unix_stream_read_skb
  mutex_lock
  skb = skb_recv_datagram
  mutex_unlock
  spin_lock
  if (oob_skb == skb) {
				unix_release_sock
				  if (u->oob_skb) {
				    kfree_skb(u->oob_skb)
				    u->oob_skb = NULL
				  }
    oob_skb = NULL
    drop = true
  }
  spin_unlock
  if (drop) {
    skb_unref(skb)
    kfree_skb(skb)
  }

In v2 I'll do what unix_stream_read_generic() does: take state lock and
check for SOCK_DEAD.

> and the below part can be like the following not to slow down
> the common case:
> 
> 	if (!skb)
> 		return err;
> 
>> +
>> +#if IS_ENABLED(CONFIG_AF_UNIX_OOB)
>> +	if (skb) {
> 
> 	if (unlikely(skb == READ_ONCE(u->oob_skb))) {
> 
> 
>> +		bool drop = false;
>> +
>> +		spin_lock(&sk->sk_receive_queue.lock);
>> +		if (skb == u->oob_skb) {
> 
> 		if (likely(skb == u->oob_skb)) {
> 
>> +			WRITE_ONCE(u->oob_skb, NULL);
>> +			drop = true;
>> +		}
>> +		spin_unlock(&sk->sk_receive_queue.lock);
>> +
>> +		if (drop) {
>> +			WARN_ON_ONCE(skb_unref(skb));
>> +			kfree_skb(skb);
>> +			skb = NULL;
>> +			err = -EAGAIN;
> 			return -EAGAIN;
> 
>> +		}
>> +	}
>> +#endif
> 
> 	return recv_actor(sk, skb);

All right, thanks. So here's v2:
https://lore.kernel.org/netdev/20240622223324.3337956-1-mhal@rbox.co/