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 18:39, Kuniyuki Iwashima wrote:
> From: Rao Shoaib <rao.shoaib@oracle.com>
> Date: Tue, 23 Apr 2024 18:18:24 -0700
>> 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.
> 
> If skb_peek_next() returns NULL, should it also fall down to the
> !skb case ?
> 
> TCP is blocked in the situation.
> 
> So, I think this hunk in unix_stream_read_generic() is not needed.

I do not understand can you please explain.

Regards,

Shoaib

> 
> ---8<---
>>>> from socket import *
>>>>
>>>> s = socket()
>>>> s.listen()
>>>>
>>>> c1 = socket()
>>>> c1.connect(s.getsockname())
>>>> c2, _ = s.accept()
>>>>
>>>> c1.send(b'h', MSG_OOB)
> 1
>>>> c2.recv(5, MSG_PEEK)
> ^C
> ---8<---