| Message ID | ZjCozXP/DBt/C8WZ@shoaib-laptop (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v4] af_unix: Read with MSG_PEEK loops if the first unread byte is OOB | expand |
From: Rao Shoaib <Rao.Shoaib@oracle.com> Date: Tue, 30 Apr 2024 01:16:13 -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 | 25 +++++++++++++------------ > 1 file changed, 13 insertions(+), 12 deletions(-) > > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c > index 9a6ad5974dff..e88ec8744329 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); My last comment for v3 was about this line. https://lore.kernel.org/netdev/20240424013921.16819-1-kuniyu@amazon.com/ Here, (flags & MSG_PEEK) is true, and if skb_peek_next() returns NULL, > } > } > } > @@ -2747,9 +2747,10 @@ 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; we will bail out the loop due to this change. However, no data is copied here, so whether we break or not should depend on MSG_DONTWAIT, which is handled in the following `if (skb == NULL)` block. In the example below, TCP socket is blocked because there is no data to receive and MSG_DONTWAIT is not specified. And this is unblocked when normal data arrives. ---8<--- >>> from socket import * >>> >>> s = socket() >>> s.listen() >>> >>> c1 = socket() >>> c1.connect(s.getsockname()) >>> >>> c2, _ = s.accept() >>> >>> c1.send(b'a', MSG_OOB) 1 >>> c2.recv(1, MSG_PEEK) ^C ---8<--- But with your patch, AF_UNIX socket is not blocked even without MSG_DONTWAIT. ---8<--- >>> from socket import * >>> >>> c1, c2 = socketpair(AF_UNIX, SOCK_STREAM, 0) >>> c1.send(b'a', MSG_OOB) 1 >>> c2.recv(1, MSG_PEEK) b'' ---8<--- That's why I said the change in unix_stream_read_generic() is not needed.
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 9a6ad5974dff..e88ec8744329 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,10 @@ 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; } } #endif
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 | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-)