From a6736a0addd60fccc3a3508461d72314cc609772 Mon Sep 17 00:00:00 2001 From: Rao Shoaib Date: Tue, 11 Jun 2024 01:46:39 -0700 Subject: af_unix: Read with MSG_PEEK loops if the first unread byte is OOB 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' >>> from socket import * >>> c1, c2 = socketpair(AF_UNIX, SOCK_STREAM) >>> c2.setsockopt(SOL_SOCKET, SO_OOBINLINE, 1) >>> c1.send(b'a', MSG_OOB) 1 >>> c1.send(b'b') 1 >>> c2.recv(1, MSG_PEEK | MSG_DONTWAIT) b'a' >>> c2.recv(1, MSG_PEEK | MSG_DONTWAIT) b'a' >>> c2.recv(1, MSG_DONTWAIT) b'a' >>> c2.recv(1, MSG_PEEK | MSG_DONTWAIT) b'b' >>> Fixes: 314001f0bf92 ("af_unix: Add OOB support") Signed-off-by: Rao Shoaib Reviewed-by: Kuniyuki Iwashima Link: https://lore.kernel.org/r/20240611084639.2248934-1-Rao.Shoaib@oracle.com Signed-off-by: Jakub Kicinski --- net/unix/af_unix.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) (limited to 'net/unix/af_unix.c') diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 80846279de9f..5e695a9a609c 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -2625,18 +2625,18 @@ 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); + unlinked_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); - unlinked_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); } } -- cgit From b94038d841a91d0e3f59cfe4d073e210910366ee Mon Sep 17 00:00:00 2001 From: Kuniyuki Iwashima Date: Mon, 24 Jun 2024 18:36:37 -0700 Subject: af_unix: Stop recv(MSG_PEEK) at consumed OOB skb. After consuming OOB data, recv() reading the preceding data must break at the OOB skb regardless of MSG_PEEK. Currently, MSG_PEEK does not stop recv() for AF_UNIX, and the behaviour is not compliant with TCP. >>> from socket import * >>> c1, c2 = socketpair(AF_UNIX) >>> c1.send(b'hello', MSG_OOB) 5 >>> c1.send(b'world') 5 >>> c2.recv(1, MSG_OOB) b'o' >>> c2.recv(9, MSG_PEEK) # This should return b'hell' b'hellworld' # even with enough buffer. Let's fix it by returning NULL for consumed skb and unlinking it only if MSG_PEEK is not specified. This patch also adds test cases that add recv(MSG_PEEK) before each recv(). Without fix: # RUN msg_oob.peek.oob_ahead_break ... # msg_oob.c:134:oob_ahead_break:AF_UNIX :hellworld # msg_oob.c:135:oob_ahead_break:Expected:hell # msg_oob.c:137:oob_ahead_break:Expected ret[0] (9) == expected_len (4) # oob_ahead_break: Test terminated by assertion # FAIL msg_oob.peek.oob_ahead_break not ok 13 msg_oob.peek.oob_ahead_break With fix: # RUN msg_oob.peek.oob_ahead_break ... # OK msg_oob.peek.oob_ahead_break ok 13 msg_oob.peek.oob_ahead_break Fixes: 314001f0bf92 ("af_unix: Add OOB support") Signed-off-by: Kuniyuki Iwashima Signed-off-by: Paolo Abeni --- net/unix/af_unix.c | 9 ++++++--- tools/testing/selftests/net/af_unix/msg_oob.c | 25 +++++++++++++++++++++++-- 2 files changed, 29 insertions(+), 5 deletions(-) (limited to 'net/unix/af_unix.c') diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 5e695a9a609c..2eaecf9d78a4 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -2613,9 +2613,12 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk, { struct unix_sock *u = unix_sk(sk); - if (!unix_skb_len(skb) && !(flags & MSG_PEEK)) { - skb_unlink(skb, &sk->sk_receive_queue); - consume_skb(skb); + if (!unix_skb_len(skb)) { + if (!(flags & MSG_PEEK)) { + skb_unlink(skb, &sk->sk_receive_queue); + consume_skb(skb); + } + skb = NULL; } else { struct sk_buff *unlinked_skb = NULL; diff --git a/tools/testing/selftests/net/af_unix/msg_oob.c b/tools/testing/selftests/net/af_unix/msg_oob.c index d427d39d0806..de8d1fcde883 100644 --- a/tools/testing/selftests/net/af_unix/msg_oob.c +++ b/tools/testing/selftests/net/af_unix/msg_oob.c @@ -21,6 +21,21 @@ FIXTURE(msg_oob) */ }; +FIXTURE_VARIANT(msg_oob) +{ + bool peek; +}; + +FIXTURE_VARIANT_ADD(msg_oob, no_peek) +{ + .peek = false, +}; + +FIXTURE_VARIANT_ADD(msg_oob, peek) +{ + .peek = true +}; + static void create_unix_socketpair(struct __test_metadata *_metadata, FIXTURE_DATA(msg_oob) *self) { @@ -156,8 +171,14 @@ static void __recvpair(struct __test_metadata *_metadata, __sendpair(_metadata, self, buf, len, flags) #define recvpair(expected_buf, expected_len, buf_len, flags) \ - __recvpair(_metadata, self, \ - expected_buf, expected_len, buf_len, flags) + do { \ + if (variant->peek) \ + __recvpair(_metadata, self, \ + expected_buf, expected_len, \ + buf_len, (flags) | MSG_PEEK); \ + __recvpair(_metadata, self, \ + expected_buf, expected_len, buf_len, flags); \ + } while (0) TEST_F(msg_oob, non_oob) { -- cgit From 93c99f21db360957d49853e5666b5c147f593bda Mon Sep 17 00:00:00 2001 From: Kuniyuki Iwashima Date: Mon, 24 Jun 2024 18:36:38 -0700 Subject: af_unix: Don't stop recv(MSG_DONTWAIT) if consumed OOB skb is at the head. Let's say a socket send()s "hello" with MSG_OOB and "world" without flags, >>> from socket import * >>> c1, c2 = socketpair(AF_UNIX) >>> c1.send(b'hello', MSG_OOB) 5 >>> c1.send(b'world') 5 and its peer recv()s "hell" and "o". >>> c2.recv(10) b'hell' >>> c2.recv(1, MSG_OOB) b'o' Now the consumed OOB skb stays at the head of recvq to return a correct value for ioctl(SIOCATMARK), which is broken now and fixed by a later patch. Then, if peer issues recv() with MSG_DONTWAIT, manage_oob() returns NULL, so recv() ends up with -EAGAIN. >>> c2.setblocking(False) # This causes -EAGAIN even with available data >>> c2.recv(5) Traceback (most recent call last): File "", line 1, in BlockingIOError: [Errno 11] Resource temporarily unavailable However, next recv() will return the following available data, "world". >>> c2.recv(5) b'world' When the consumed OOB skb is at the head of the queue, we need to fetch the next skb to fix the weird behaviour. Note that the issue does not happen without MSG_DONTWAIT because we can retry after manage_oob(). This patch also adds a test case that covers the issue. Without fix: # RUN msg_oob.no_peek.ex_oob_break ... # msg_oob.c:134:ex_oob_break:AF_UNIX :Resource temporarily unavailable # msg_oob.c:135:ex_oob_break:Expected:ld # msg_oob.c:137:ex_oob_break:Expected ret[0] (-1) == expected_len (2) # ex_oob_break: Test terminated by assertion # FAIL msg_oob.no_peek.ex_oob_break not ok 8 msg_oob.no_peek.ex_oob_break With fix: # RUN msg_oob.no_peek.ex_oob_break ... # OK msg_oob.no_peek.ex_oob_break ok 8 msg_oob.no_peek.ex_oob_break Fixes: 314001f0bf92 ("af_unix: Add OOB support") Signed-off-by: Kuniyuki Iwashima Signed-off-by: Paolo Abeni --- net/unix/af_unix.c | 19 +++++++++++++++---- tools/testing/selftests/net/af_unix/msg_oob.c | 11 +++++++++++ 2 files changed, 26 insertions(+), 4 deletions(-) (limited to 'net/unix/af_unix.c') diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 2eaecf9d78a4..b0b97f8d0d09 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -2614,12 +2614,23 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk, struct unix_sock *u = unix_sk(sk); if (!unix_skb_len(skb)) { - if (!(flags & MSG_PEEK)) { - skb_unlink(skb, &sk->sk_receive_queue); - consume_skb(skb); + struct sk_buff *unlinked_skb = NULL; + + spin_lock(&sk->sk_receive_queue.lock); + + if (copied) { + skb = NULL; + } else if (flags & MSG_PEEK) { + skb = skb_peek_next(skb, &sk->sk_receive_queue); + } else { + unlinked_skb = skb; + skb = skb_peek_next(skb, &sk->sk_receive_queue); + __skb_unlink(unlinked_skb, &sk->sk_receive_queue); } - skb = NULL; + spin_unlock(&sk->sk_receive_queue.lock); + + consume_skb(unlinked_skb); } else { struct sk_buff *unlinked_skb = NULL; diff --git a/tools/testing/selftests/net/af_unix/msg_oob.c b/tools/testing/selftests/net/af_unix/msg_oob.c index de8d1fcde883..b5226ccec3ec 100644 --- a/tools/testing/selftests/net/af_unix/msg_oob.c +++ b/tools/testing/selftests/net/af_unix/msg_oob.c @@ -238,4 +238,15 @@ TEST_F(msg_oob, oob_break_drop) recvpair("", -EINVAL, 1, MSG_OOB); } +TEST_F(msg_oob, ex_oob_break) +{ + sendpair("hello", 5, MSG_OOB); + sendpair("wor", 3, MSG_OOB); + sendpair("ld", 2, 0); + + recvpair("hellowo", 7, 10, 0); /* Break at OOB but not at ex-OOB. */ + recvpair("r", 1, 1, MSG_OOB); + recvpair("ld", 2, 2, 0); +} + TEST_HARNESS_MAIN -- cgit From 36893ef0b661671ee64eb37bf5f345f33d2cabb7 Mon Sep 17 00:00:00 2001 From: Kuniyuki Iwashima Date: Mon, 24 Jun 2024 18:36:40 -0700 Subject: af_unix: Don't stop recv() at consumed ex-OOB skb. Currently, recv() is stopped at a consumed OOB skb even if a new OOB skb is queued and we can ignore the old OOB skb. >>> from socket import * >>> c1, c2 = socket(AF_UNIX, SOCK_STREAM) >>> c1.send(b'hellowor', MSG_OOB) 8 >>> c2.recv(1, MSG_OOB) # consume OOB data stays at middle of recvq. b'r' >>> c1.send(b'ld', MSG_OOB) 2 >>> c2.recv(10) # recv() stops at the old consumed OOB b'hellowo' # should be 'hellowol' manage_oob() should not stop recv() at the old consumed OOB skb if there is a new OOB data queued. Note that TCP behaviour is apparently wrong in this test case because we can recv() the same OOB data twice. Without fix: # RUN msg_oob.no_peek.ex_oob_ahead_break ... # msg_oob.c:138:ex_oob_ahead_break:AF_UNIX :hellowo # msg_oob.c:139:ex_oob_ahead_break:Expected:hellowol # msg_oob.c:141:ex_oob_ahead_break:Expected ret[0] (7) == expected_len (8) # ex_oob_ahead_break: Test terminated by assertion # FAIL msg_oob.no_peek.ex_oob_ahead_break not ok 11 msg_oob.no_peek.ex_oob_ahead_break With fix: # RUN msg_oob.no_peek.ex_oob_ahead_break ... # msg_oob.c:146:ex_oob_ahead_break:AF_UNIX :hellowol # msg_oob.c:147:ex_oob_ahead_break:TCP :helloworl # OK msg_oob.no_peek.ex_oob_ahead_break ok 11 msg_oob.no_peek.ex_oob_ahead_break Fixes: 314001f0bf92 ("af_unix: Add OOB support") Signed-off-by: Kuniyuki Iwashima Signed-off-by: Paolo Abeni --- net/unix/af_unix.c | 2 +- tools/testing/selftests/net/af_unix/msg_oob.c | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) (limited to 'net/unix/af_unix.c') diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index b0b97f8d0d09..07f5eaa04b5b 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -2618,7 +2618,7 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk, spin_lock(&sk->sk_receive_queue.lock); - if (copied) { + if (copied && (!u->oob_skb || skb == u->oob_skb)) { skb = NULL; } else if (flags & MSG_PEEK) { skb = skb_peek_next(skb, &sk->sk_receive_queue); diff --git a/tools/testing/selftests/net/af_unix/msg_oob.c b/tools/testing/selftests/net/af_unix/msg_oob.c index 46e92d06b0a3..acf4bd0afe17 100644 --- a/tools/testing/selftests/net/af_unix/msg_oob.c +++ b/tools/testing/selftests/net/af_unix/msg_oob.c @@ -288,4 +288,20 @@ TEST_F(msg_oob, ex_oob_drop_2) } } +TEST_F(msg_oob, ex_oob_ahead_break) +{ + sendpair("hello", 5, MSG_OOB); + sendpair("wor", 3, MSG_OOB); + + recvpair("r", 1, 1, MSG_OOB); + + sendpair("ld", 2, MSG_OOB); + + tcp_incompliant { + recvpair("hellowol", 8, 10, 0); /* TCP recv()s "helloworl", why "r" ?? */ + } + + recvpair("d", 1, 1, MSG_OOB); +} + TEST_HARNESS_MAIN -- cgit From e400cfa38bb0419cf1313e5494ea2b7d114e86d7 Mon Sep 17 00:00:00 2001 From: Kuniyuki Iwashima Date: Mon, 24 Jun 2024 18:36:44 -0700 Subject: af_unix: Fix wrong ioctl(SIOCATMARK) when consumed OOB skb is at the head. Even if OOB data is recv()ed, ioctl(SIOCATMARK) must return 1 when the OOB skb is at the head of the receive queue and no new OOB data is queued. Without fix: # RUN msg_oob.no_peek.oob ... # msg_oob.c:305:oob:Expected answ[0] (0) == oob_head (1) # oob: Test terminated by assertion # FAIL msg_oob.no_peek.oob not ok 2 msg_oob.no_peek.oob With fix: # RUN msg_oob.no_peek.oob ... # OK msg_oob.no_peek.oob ok 2 msg_oob.no_peek.oob Fixes: 314001f0bf92 ("af_unix: Add OOB support") Signed-off-by: Kuniyuki Iwashima Signed-off-by: Paolo Abeni --- net/unix/af_unix.c | 15 +++++- tools/testing/selftests/net/af_unix/msg_oob.c | 68 +++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 2 deletions(-) (limited to 'net/unix/af_unix.c') diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 07f5eaa04b5b..142f56770b77 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -3107,12 +3107,23 @@ static int unix_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg) #if IS_ENABLED(CONFIG_AF_UNIX_OOB) case SIOCATMARK: { + struct unix_sock *u = unix_sk(sk); struct sk_buff *skb; int answ = 0; + mutex_lock(&u->iolock); + skb = skb_peek(&sk->sk_receive_queue); - if (skb && skb == READ_ONCE(unix_sk(sk)->oob_skb)) - answ = 1; + if (skb) { + struct sk_buff *oob_skb = READ_ONCE(u->oob_skb); + + if (skb == oob_skb || + (!oob_skb && !unix_skb_len(skb))) + answ = 1; + } + + mutex_unlock(&u->iolock); + err = put_user(answ, (int __user *)arg); } break; diff --git a/tools/testing/selftests/net/af_unix/msg_oob.c b/tools/testing/selftests/net/af_unix/msg_oob.c index 28b09b36a2f1..2d0024329437 100644 --- a/tools/testing/selftests/net/af_unix/msg_oob.c +++ b/tools/testing/selftests/net/af_unix/msg_oob.c @@ -288,6 +288,26 @@ static void __setinlinepair(struct __test_metadata *_metadata, } } +static void __siocatmarkpair(struct __test_metadata *_metadata, + FIXTURE_DATA(msg_oob) *self, + bool oob_head) +{ + int answ[2] = {}; + int i; + + for (i = 0; i < 2; i++) { + int ret; + + ret = ioctl(self->fd[i * 2 + 1], SIOCATMARK, &answ[i]); + ASSERT_EQ(ret, 0); + } + + ASSERT_EQ(answ[0], oob_head); + + if (self->tcp_compliant) + ASSERT_EQ(answ[0], answ[1]); +} + #define sendpair(buf, len, flags) \ __sendpair(_metadata, self, buf, len, flags) @@ -304,6 +324,9 @@ static void __setinlinepair(struct __test_metadata *_metadata, #define epollpair(oob_remaining) \ __epollpair(_metadata, self, oob_remaining) +#define siocatmarkpair(oob_head) \ + __siocatmarkpair(_metadata, self, oob_head) + #define setinlinepair() \ __setinlinepair(_metadata, self) @@ -325,9 +348,11 @@ TEST_F(msg_oob, oob) { sendpair("x", 1, MSG_OOB); epollpair(true); + siocatmarkpair(true); recvpair("x", 1, 1, MSG_OOB); epollpair(false); + siocatmarkpair(true); } TEST_F(msg_oob, oob_drop) @@ -481,18 +506,40 @@ TEST_F(msg_oob, ex_oob_ahead_break) epollpair(false); } +TEST_F(msg_oob, ex_oob_siocatmark) +{ + sendpair("hello", 5, MSG_OOB); + epollpair(true); + siocatmarkpair(false); + + recvpair("o", 1, 1, MSG_OOB); + epollpair(false); + siocatmarkpair(false); + + sendpair("world", 5, MSG_OOB); + epollpair(true); + siocatmarkpair(false); + + recvpair("hell", 4, 4, 0); /* Intentionally stop at ex-OOB. */ + epollpair(true); + siocatmarkpair(false); +} + TEST_F(msg_oob, inline_oob) { setinlinepair(); sendpair("x", 1, MSG_OOB); epollpair(true); + siocatmarkpair(true); recvpair("", -EINVAL, 1, MSG_OOB); epollpair(true); + siocatmarkpair(true); recvpair("x", 1, 1, 0); epollpair(false); + siocatmarkpair(false); } TEST_F(msg_oob, inline_oob_break) @@ -591,4 +638,25 @@ TEST_F(msg_oob, inline_ex_oob_drop) } } +TEST_F(msg_oob, inline_ex_oob_siocatmark) +{ + sendpair("hello", 5, MSG_OOB); + epollpair(true); + siocatmarkpair(false); + + recvpair("o", 1, 1, MSG_OOB); + epollpair(false); + siocatmarkpair(false); + + setinlinepair(); + + sendpair("world", 5, MSG_OOB); + epollpair(true); + siocatmarkpair(false); + + recvpair("hell", 4, 4, 0); /* Intentionally stop at ex-OOB. */ + epollpair(true); + siocatmarkpair(false); +} + TEST_HARNESS_MAIN -- cgit