* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.