Re: [PATCH net v3] af_unix: Read with MSG_PEEK loops if the first unread byte is OOB

[Rao Shoaib <rao.shoaib@oracle.com>]

On 4/23/24 17:15, Kuniyuki Iwashima wrote:
> From: Rao Shoaib <Rao.Shoaib@oracle.com>
> Date: Mon, 22 Apr 2024 02:25:03 -0700
>> Read with MSG_PEEK flag loops if the first byte to read is an OOB byte.
>> commit 22dd70eb2c3d ("af_unix: Don't peek OOB data without MSG_OOB.")
>> addresses the loop issue but does not address the issue that no data
>> beyond OOB byte can be read.
>>
>>>>> from socket import *
>>>>> c1, c2 = socketpair(AF_UNIX, SOCK_STREAM)
>>>>> c1.send(b'a', MSG_OOB)
>> 1
>>>>> c1.send(b'b')
>> 1
>>>>> c2.recv(1, MSG_PEEK | MSG_DONTWAIT)
>> b'b'
>>
>> Fixes: 314001f0bf92 ("af_unix: Add OOB support")
>> Signed-off-by: Rao Shoaib <Rao.Shoaib@oracle.com>
>> ---
>>  net/unix/af_unix.c | 26 ++++++++++++++------------
>>  1 file changed, 14 insertions(+), 12 deletions(-)
>>
>> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
>> index 9a6ad5974dff..ed5f70735435 100644
>> --- a/net/unix/af_unix.c
>> +++ b/net/unix/af_unix.c
>> @@ -2658,19 +2658,19 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk,
>>  		if (skb == u->oob_skb) {
>>  			if (copied) {
>>  				skb = NULL;
>> -			} else if (sock_flag(sk, SOCK_URGINLINE)) {
>> -				if (!(flags & MSG_PEEK)) {
>> +			} else if (!(flags & MSG_PEEK)) {
>> +				if (sock_flag(sk, SOCK_URGINLINE)) {
>>  					WRITE_ONCE(u->oob_skb, NULL);
>>  					consume_skb(skb);
>> +				} else {
>> +					skb_unlink(skb, &sk->sk_receive_queue);
>> +					WRITE_ONCE(u->oob_skb, NULL);
>> +					if (!WARN_ON_ONCE(skb_unref(skb)))
>> +						kfree_skb(skb);
>> +					skb = skb_peek(&sk->sk_receive_queue);
> 
> I added a comment about this case.

OK. I will sync up.
> 
> 
>>  				}
>> -			} else if (flags & MSG_PEEK) {
>> -				skb = NULL;
>> -			} else {
>> -				skb_unlink(skb, &sk->sk_receive_queue);
>> -				WRITE_ONCE(u->oob_skb, NULL);
>> -				if (!WARN_ON_ONCE(skb_unref(skb)))
>> -					kfree_skb(skb);
>> -				skb = skb_peek(&sk->sk_receive_queue);
>> +			} else if (!sock_flag(sk, SOCK_URGINLINE)) {
>> +				skb = skb_peek_next(skb, &sk->sk_receive_queue);
>>  			}
>>  		}
>>  	}
>> @@ -2747,9 +2747,11 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
>>  #if IS_ENABLED(CONFIG_AF_UNIX_OOB)
>>  		if (skb) {
>>  			skb = manage_oob(skb, sk, flags, copied);
>> -			if (!skb && copied) {
>> +			if (!skb) {
>>  				unix_state_unlock(sk);
>> -				break;
>> +				if (copied || (flags & MSG_PEEK))
>> +					break;
>> +				goto redo;
> 
> Here, copied == 0 && !(flags & MSG_PEEK) && skb == NULL, so it means
> skb_peek(&sk->sk_receive_queue) above returned NULL.  Then, we need
> not jump to the redo label, where we call the same skb_peek().
> 
> Instead, we can just fall through the if (!skb) clause below.
> 
> Thanks!

Yes that makes sense. I will submit a new version with the jump to redo
removed.

Thanks,

Shoaib

> 
>>  			}
>>  		}
>>  #endif
>> -- 
>> 2.39.3