[PATCH v1 net 0/3] af_unix: Minor fixes for MSG_OOB and MSG_PEEK.

[PATCH v1 net 0/3] af_unix: Minor fixes for MSG_OOB and MSG_PEEK.

[Kuniyuki Iwashima]

Fahad Alharbi reported blocking recv(MSG_PEEK) could hog CPU
due to OOB skb.

Patch 1 and 2 fixes the issues and Patch 3 adds tests.


Kuniyuki Iwashima (3):
  af_unix: Update last skb marker in manage_oob().
  af_unix: Return immediately when manage_oob() returns NULL for
    0-length buffer.
  selftest: af_unix: Add zero-buffer test for msg_oob.c

 net/unix/af_unix.c                            | 12 ++--
 tools/testing/selftests/net/af_unix/msg_oob.c | 67 +++++++++++++++++++
 2 files changed, 75 insertions(+), 4 deletions(-)

-- 
2.55.0.970.g62bdec98f9-goog

[PATCH v1 net 1/3] af_unix: Update last skb marker in manage_oob().

[Kuniyuki Iwashima]

Fahad Alharbi reported that blocking recv(MSG_PEEK) could hog CPU
due to OOB skb.

In the following cases, manage_oob() skips OOB skb(s) and returns
NULL for the last recv(MSG_PEEK):

  socketpair(AF_UNIX, SOCK_STREAM, 0, sk);

  1) skb -> OOB skb -> NULL
     send(sk[0], "ab", 2, MSG_OOB);
     recv(sk[1], buf, 0, MSG_PEEK);

  2) skb -> consumed OOB skb -> NULL
     send(sk[0], "ab", 2, MSG_OOB);
     recv(sk[1], buf, 1, MSG_OOB);
     recv(sk[1], buf, 0, MSG_PEEK);

  3) consumed OOB skb -> OOB skb -> NULL
     send(sk[0], "a", 1, MSG_OOB);
     recv(sk[1], buf, 0, MSG_OOB);
     send(sk[0], "b", 1, MSG_OOB);
     recv(sk[1], buf, 1, MSG_PEEK);

Then, @copied is 0 in unix_stream_read_generic() (zero-length buffer,
or non-OOB skb is not yet consumed), and unix_stream_data_wait() is
called.

However, it returns immediately because @last is not updated in
unix_stream_read_generic(), and the thread busy-waits for a new skb.

Let's update @last in manage_oob().

For MSG_PEEK, @last is updated with the skipped OOB, and for the
non-peek case, @last matches the returned value (when !copied)
because OOB is unlinked.

Note that manage_oob() is inlined and no stack canary is added.

Fixes: 22dd70eb2c3d ("af_unix: Don't peek OOB data without MSG_OOB.")
Reported-by: Fahad Alharbi <fahad@codepure.com>
Signed-off-by: Kuniyuki Iwashima <kuniyu@google.com>
---
 net/unix/af_unix.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 13f9926bf205..6861370062df 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2812,8 +2812,8 @@ static int unix_stream_recv_urg(struct unix_stream_read_state *state)
 	return 1;
 }
 
-static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk,
-				  int flags, int copied)
+static struct sk_buff *manage_oob(struct sk_buff *skb, struct sk_buff **last,
+				  struct sock *sk, int flags, int copied)
 {
 	struct sk_buff *read_skb = NULL, *unread_skb = NULL;
 	struct unix_sock *u = unix_sk(sk);
@@ -2827,11 +2827,13 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk,
 		if (copied && (!u->oob_skb || skb == u->oob_skb)) {
 			skb = NULL;
 		} else if (flags & MSG_PEEK) {
+			*last = skb;
 			skb = skb_peek_next(skb, &sk->sk_receive_queue);
 		} else {
 			read_skb = skb;
 			skb = skb_peek_next(skb, &sk->sk_receive_queue);
 			__skb_unlink(read_skb, &sk->sk_receive_queue);
+			*last = skb;
 		}
 
 		if (!skb)
@@ -2850,8 +2852,10 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk,
 			__skb_unlink(skb, &sk->sk_receive_queue);
 			unread_skb = skb;
 			skb = skb_peek(&sk->sk_receive_queue);
+			*last = skb;
 		}
 	} else if (!sock_flag(sk, SOCK_URGINLINE)) {
+		*last = skb;
 		skb = skb_peek_next(skb, &sk->sk_receive_queue);
 	}
 
@@ -2971,7 +2975,7 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
 again:
 #if IS_ENABLED(CONFIG_AF_UNIX_OOB)
 		if (skb) {
-			skb = manage_oob(skb, sk, flags, copied);
+			skb = manage_oob(skb, &last, sk, flags, copied);
 			if (!skb && copied) {
 				unix_state_unlock(sk);
 				break;
-- 
2.55.0.970.g62bdec98f9-goog

[PATCH v1 net 2/3] af_unix: Return immediately when manage_oob() returns NULL for 0-length buffer.

[Kuniyuki Iwashima]

Fahad Alharbi reported that recv(0, MSG_PEEK) triggers busy-wait
in unix_stream_read_generic() if recv() is blocking and the last
skb in the queue is MSG_OOB skb.

In such a situation, TCP returns 0 immediately regardless of
blocking or non-blocking.

Let's follow the behaviour.

Fixes: 314001f0bf92 ("af_unix: Add OOB support")
Reported-by: Fahad Alharbi <fahad@codepure.com>
Signed-off-by: Kuniyuki Iwashima <kuniyu@google.com>
---
 net/unix/af_unix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 6861370062df..2da1017f8873 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2976,7 +2976,7 @@ 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, &last, sk, flags, copied);
-			if (!skb && copied) {
+			if (!skb && (copied || !state->size)) {
 				unix_state_unlock(sk);
 				break;
 			}
-- 
2.55.0.970.g62bdec98f9-goog

[PATCH v1 net 3/3] selftest: af_unix: Add zero-buffer test for msg_oob.c

[Kuniyuki Iwashima]

The previous patches fixed two issues related to zero-length
buffer with MSG_PEEK for MSG_OOB skb.

Let's add corresponding tests in msg_oob.c.

Without this series:

  # FAILED: 50 / 60 tests passed.
  # Totals: pass:50 fail:10 xfail:0 xpass:0 skip:0 error:0

With this series:

  # PASSED: 60 / 60 tests passed.
  # Totals: pass:60 fail:0 xfail:0 xpass:0 skip:0 error:0

Signed-off-by: Kuniyuki Iwashima <kuniyu@google.com>
---
 tools/testing/selftests/net/af_unix/msg_oob.c | 67 +++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/tools/testing/selftests/net/af_unix/msg_oob.c b/tools/testing/selftests/net/af_unix/msg_oob.c
index 1b499d56656c..f051d79f7a8e 100644
--- a/tools/testing/selftests/net/af_unix/msg_oob.c
+++ b/tools/testing/selftests/net/af_unix/msg_oob.c
@@ -290,6 +290,25 @@ static void __setinlinepair(struct __test_metadata *_metadata,
 	}
 }
 
+static void __setblockingpair(struct __test_metadata *_metadata,
+			      FIXTURE_DATA(msg_oob) *self)
+{
+	int i;
+
+	for (i = 0; i < 2; i++) {
+		int ret, old_flags, flags;
+
+		old_flags = fcntl(self->fd[i * 2 + 1], F_GETFL, 0);
+		ASSERT_NE(-1, old_flags);
+
+		ret = fcntl(self->fd[i * 2 + 1], F_SETFL, old_flags & ~O_NONBLOCK);
+		ASSERT_EQ(0, ret);
+
+		flags = fcntl(self->fd[i * 2 + 1], F_GETFL, 0);
+		ASSERT_EQ(old_flags & ~O_NONBLOCK, flags);
+	}
+}
+
 static void __siocatmarkpair(struct __test_metadata *_metadata,
 			     FIXTURE_DATA(msg_oob) *self,
 			     bool oob_head)
@@ -347,6 +366,9 @@ static void __resetpair(struct __test_metadata *_metadata,
 #define setinlinepair()							\
 	__setinlinepair(_metadata, self)
 
+#define setblockingpair()						\
+	__setblockingpair(_metadata, self)
+
 #define resetpair(reset)						\
 	__resetpair(_metadata, self, variant, reset)
 
@@ -888,4 +910,49 @@ TEST_F(msg_oob, inline_ex_oob_siocatmark)
 	resetpair(true);
 }
 
+TEST_F(msg_oob, zero_buf_oob)
+{
+	sendpair("a", 1, MSG_OOB);
+	recvpair("", 0, 0, 0);
+}
+
+TEST_F(msg_oob, zero_buf_oob_blocking)
+{
+	sendpair("a", 1, MSG_OOB);
+	setblockingpair();
+	recvpair("", 0, 0, 0);
+}
+
+TEST_F(msg_oob, zero_buf_non_oob_oob)
+{
+	sendpair("ab", 2, MSG_OOB);
+	recvpair("", 0, 0, 0);
+}
+
+TEST_F(msg_oob, zero_buf_non_oob_oob_blocking)
+{
+	sendpair("ab", 2, MSG_OOB);
+	setblockingpair();
+	recvpair("", 0, 0, 0);
+}
+
+TEST_F(msg_oob, zero_buf_ex_oob_oob)
+{
+	sendpair("a", 1, MSG_OOB);
+	recvpair("a", 1, 1, MSG_OOB);
+
+	sendpair("b", 1, MSG_OOB);
+	recvpair("", 0, 0, 0);
+}
+
+TEST_F(msg_oob, zero_buf_ex_oob_oob_blocking)
+{
+	sendpair("a", 1, MSG_OOB);
+	recvpair("a", 1, 1, MSG_OOB);
+
+	sendpair("b", 1, MSG_OOB);
+	setblockingpair();
+	recvpair("", 0, 0, 0);
+}
+
 TEST_HARNESS_MAIN
-- 
2.55.0.970.g62bdec98f9-goog