* [PATCH net v3] af_unix: Read with MSG_PEEK loops if the first unread byte is OOB
@ 2024-04-22 9:25 Rao Shoaib
2024-04-24 0:15 ` Kuniyuki Iwashima
0 siblings, 1 reply; 5+ messages in thread
From: Rao Shoaib @ 2024-04-22 9:25 UTC (permalink / raw)
To: davem, edumazet, kuba, pabeni; +Cc: kuniyu, netdev, Rao Shoaib
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);
}
- } 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;
}
}
#endif
--
2.39.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net v3] af_unix: Read with MSG_PEEK loops if the first unread byte is OOB
2024-04-22 9:25 [PATCH net v3] af_unix: Read with MSG_PEEK loops if the first unread byte is OOB Rao Shoaib
@ 2024-04-24 0:15 ` Kuniyuki Iwashima
2024-04-24 1:18 ` Rao Shoaib
0 siblings, 1 reply; 5+ messages in thread
From: Kuniyuki Iwashima @ 2024-04-24 0:15 UTC (permalink / raw)
To: rao.shoaib; +Cc: davem, edumazet, kuba, kuniyu, netdev, pabeni
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.
> }
> - } 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!
> }
> }
> #endif
> --
> 2.39.3
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net v3] af_unix: Read with MSG_PEEK loops if the first unread byte is OOB
2024-04-24 0:15 ` Kuniyuki Iwashima
@ 2024-04-24 1:18 ` Rao Shoaib
2024-04-24 1:39 ` Kuniyuki Iwashima
0 siblings, 1 reply; 5+ messages in thread
From: Rao Shoaib @ 2024-04-24 1:18 UTC (permalink / raw)
To: Kuniyuki Iwashima; +Cc: davem, edumazet, kuba, netdev, pabeni
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net v3] af_unix: Read with MSG_PEEK loops if the first unread byte is OOB
2024-04-24 1:18 ` Rao Shoaib
@ 2024-04-24 1:39 ` Kuniyuki Iwashima
2024-04-30 8:26 ` Rao Shoaib
0 siblings, 1 reply; 5+ messages in thread
From: Kuniyuki Iwashima @ 2024-04-24 1:39 UTC (permalink / raw)
To: rao.shoaib; +Cc: davem, edumazet, kuba, kuniyu, netdev, pabeni
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.
---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<---
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net v3] af_unix: Read with MSG_PEEK loops if the first unread byte is OOB
2024-04-24 1:39 ` Kuniyuki Iwashima
@ 2024-04-30 8:26 ` Rao Shoaib
0 siblings, 0 replies; 5+ messages in thread
From: Rao Shoaib @ 2024-04-30 8:26 UTC (permalink / raw)
To: Kuniyuki Iwashima; +Cc: davem, edumazet, kuba, netdev, pabeni
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<---
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-04-30 8:26 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-22 9:25 [PATCH net v3] af_unix: Read with MSG_PEEK loops if the first unread byte is OOB Rao Shoaib
2024-04-24 0:15 ` Kuniyuki Iwashima
2024-04-24 1:18 ` Rao Shoaib
2024-04-24 1:39 ` Kuniyuki Iwashima
2024-04-30 8:26 ` Rao Shoaib
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).