[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

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 (2) · Low: 0

- [Medium] With only this patch applied, a zero-length receive that hits
  the OOB-skip path turns a busy-wait into an indefinite block.

Pre-existing issues:
- [Medium] manage_oob() (net/unix/af_unix.c:2815-2869) drops the
  unconsumed OOB skb without decrementing u->inq_len, so SIOCINQ /…
- [Medium] The wait marker @last passed to unix_stream_data_wait() is a
  bare skb address captured under unix_state_lock, but both…

--

af_unix: Update last skb marker in manage_oob().

manage_oob() can skip OOB skbs and return NULL, after which
unix_stream_read_generic() may call unix_stream_data_wait() with a stale
@last, so the wait returns immediately and the reader busy-waits.  This
patch passes @last into manage_oob() and updates it on the paths that skip
or unlink an OOB skb.

> 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;
>  		}

[Severity: Medium]

This is a pre-existing issue and not something this patch introduces, but
since this branch is being touched: the OOB skb is unlinked here and then
freed at the end of manage_oob():

	kfree_skb_reason(unread_skb, SKB_DROP_REASON_UNIX_SKIP_OOB);

I don't see u->inq_len being adjusted anywhere in manage_oob(), while
queue_oob() accounts the 1-byte OOB skb:

	WRITE_ONCE(ousk->inq_len, ousk->inq_len + 1);

Other places that remove queued bytes do decrement it, including
unix_stream_read_skb(), which drops the same skb with the same reason:

	WRITE_ONCE(u->inq_len, u->inq_len - unix_skb_len(skb));
	if (skb == u->oob_skb) {
		WRITE_ONCE(u->oob_skb, NULL);
		...
		kfree_skb_reason(skb, SKB_DROP_REASON_UNIX_SKIP_OOB);

With:

	send(sk0, "ab", 2, MSG_OOB);
	recv(sk1, buf, 1, 0);	/* reads 'a', inq_len == 1 */
	recv(sk1, buf, 10, 0);	/* manage_oob() unlinks and frees the OOB skb */

the receive queue ends up empty with u->inq_len still 1.  Does SIOCINQ via
unix_inq_len(), and msg_inq/SCM_INQ, then report a readable byte that can
never be read, growing by one for every skipped OOB byte?

>  	} else if (!sock_flag(sk, SOCK_URGINLINE)) {
> +		*last = skb;
>  		skb = skb_peek_next(skb, &sk->sk_receive_queue);
>  	}
>  

[Severity: Medium]

This is a pre-existing property of the @last marker rather than something
introduced here, but this path now publishes u->oob_skb as the wait marker,
and @last is only ever compared as a bare address:

unix_stream_data_wait()
	tail = skb_peek_tail(&sk->sk_receive_queue);
	if (tail != last || ...)
		break;

unix_stream_read_generic() drops both unix_state_lock and u->iolock before
that comparison:

	unix_state_unlock(sk);
	...
	mutex_unlock(&u->iolock);

	timeo = unix_stream_data_wait(sk, timeo, last, freezable);

so a second reader on the same socket can free that OOB skb in manage_oob()
via kfree_skb_reason(unread_skb, SKB_DROP_REASON_UNIX_SKIP_OOB).

If the sk_buff slab hands the same address back for a newly sent skb that
becomes the queue tail before the sleeper reaches prepare_to_wait(), the
accompanying sk_data_ready() finds no waiter and the tail re-check sees
tail == last again.

The pointer is only compared and never dereferenced, so this is not a
use-after-free, but can the recv() then sleep with readable data queued
until the next send, a signal, or SO_RCVTIMEO?  Would comparing a tail
generation counter or qlen be more robust than a raw skb address?

> @@ -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;

[Severity: Medium]

For cases 1) and 2) in the commit message, recv(sk[1], buf, 0, MSG_PEEK)
leaves copied == 0 because size is 0, so this escape is not taken when
manage_oob() returns NULL.

Execution then falls into the skb == NULL path, where copied >= target is
0 >= 1 since sock_rcvlowat() clamps target to at least 1, and
unix_stream_data_wait() is entered with @last now set to the skipped OOB
skb, which is also the queue tail.

Since tail == last now holds, does the zero-length blocking peek sleep
until unrelated new data arrives, a signal is delivered, or SO_RCVTIMEO
expires, instead of returning 0?  With MSG_DONTWAIT the same path appears
to return -EAGAIN rather than 0.

The next patch in the series, "af_unix: Return immediately when
manage_oob() returns NULL for 0-length buffer.", changes this test to:

	if (!skb && (copied || !state->size)) {

which looks like it covers exactly this case.  Would it make sense to order
that change first, or fold the two together, so that a bisection landing on
this commit alone does not convert the busy-wait into a blocking recv()?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902202202.892676-1-kuniyu%40google.com

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] Zero-size early return is placed after the queue-mutating
  manage_oob() call: recv(fd, buf, 0, 0) (zero-length, no MSG_PEEK) on…

--

af_unix: Return immediately when manage_oob() returns NULL for 0-length buffer.

This makes unix_stream_read_generic() break out of the receive loop when
manage_oob() returns NULL and the user supplied a zero-length buffer, so a
blocking recv() with a trailing MSG_OOB skb no longer busy-waits.  The stated
goal is to match TCP, which returns 0 immediately in that situation.

> 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)) {

[Severity: Medium]
The new !state->size condition applies to every zero-length read, not only
the MSG_PEEK case described in the commit message.  Since it is evaluated
after manage_oob() has already run, does a plain recv(fd, buf, 0, 0) now
destroy the pending OOB byte and still report success?

With queue == [oob skb] and SO_OOBINLINE off, manage_oob() takes the
non-peek branch:

net/unix/af_unix.c:manage_oob() {
	...
	} else if (!(flags & MSG_PEEK)) {
		WRITE_ONCE(u->oob_skb, NULL);

		if (!sock_flag(sk, SOCK_URGINLINE)) {
			__skb_unlink(skb, &sk->sk_receive_queue);
			unread_skb = skb;
			skb = skb_peek(&sk->sk_receive_queue);
			*last = skb;
		}
	...
unlock:
	...
	kfree_skb_reason(unread_skb, SKB_DROP_REASON_UNIX_SKIP_OOB);

	return skb;
}

so u->oob_skb is cleared and the byte is freed before the caller ever looks
at state->size.  The loop then breaks and unix_stream_read_generic() returns
0, after which recv(MSG_OOB) returns -EINVAL and SIOCATMARK/EPOLLPRI report
no urgent data.

The commit message says:

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

    Let's follow the behaviour.

Is that comparison exact for the non-peek case?  In tcp_recvmsg_locked() the
found_ok_skb path computes:

net/ipv4/tcp.c:tcp_recvmsg_locked() {
	...
	used = skb->len - offset;
	if (len < used)
		used = len;

	if (unlikely(tp->urg_data)) {
		u32 urg_offset = tp->urg_seq - *seq;
		if (urg_offset < used) {
	...
}

With len == 0 used is 0, so the urgent-skip is not taken and tp->urg_data
survives the zero-length read.  TCP returns 0 without consuming or
invalidating the urgent byte, while AF_UNIX now returns 0 having dropped it.

Would it be equivalent, and non-destructive, to test the zero-length case
before manage_oob() mutates the queue, e.g. break out when
skb && !state->size, so the empty-queue case still blocks the same way as
TCP?

One more note on the selftest added later in this series:

tools/testing/selftests/net/af_unix/msg_oob.c:zero_buf_oob() {
	sendpair("a", 1, MSG_OOB);
	recvpair("", 0, 0, 0);
}

This exercises exactly the destructive non-peek path, but only compares
return values with TCP, so the difference in the resulting socket state is
not detected.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902202202.892676-1-kuniyu%40google.com

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 3 · Low: 0

- [Medium] The six new zero-buffer tests
  (tools/testing/selftests/net/af_unix/msg_oob.c:913-955) assert only…
- [Medium] The new tests do not exercise the last-skb marker updates
  added by the companion commit "af_unix: Update last skb marker in…
- [Medium] Incomplete test matrix: Missing coverage for Case 2 enumerated
  in the corresponding core patch.

--

selftest: af_unix: Add zero-buffer test for msg_oob.c

This adds a __setblockingpair() helper plus six new tests that send an
MSG_OOB byte and then perform a zero-length receive, in both non-blocking
and blocking mode.  The tests are meant to cover the two zero-length
buffer / MSG_PEEK fixes from the earlier patches in the series.

No Link: tags in the commit header.

> 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

[ ... ]

> @@ -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);
> +}

[Severity: Medium]

Should these new tests also check the OOB state with epollpair() and
siocatmarkpair() the way the rest of the file does?  Every other test in
msg_oob.c (for example oob() and inline_ex_oob_siocatmark()) brackets each
send/recv with epollpair()/siocatmarkpair() and ends with resetpair(), while
zero_buf_oob() through zero_buf_ex_oob_oob_blocking() assert only the recv()
return value.

Because of that, do these tests pass whether or not the zero-length read
consumed the OOB byte?  For the non-peek pass of recvpair("", 0, 0, 0),
unix_stream_read_generic() calls manage_oob() with copied == 0 and no
MSG_PEEK, which takes:

net/unix/af_unix.c:manage_oob() {
	...
	} else if (!(flags & MSG_PEEK)) {
		WRITE_ONCE(u->oob_skb, NULL);

		if (!sock_flag(sk, SOCK_URGINLINE)) {
			__skb_unlink(skb, &sk->sk_receive_queue);
			unread_skb = skb;
			skb = skb_peek(&sk->sk_receive_queue);
			*last = skb;
		}
	...
}

and the unlinked skb is then dropped with
kfree_skb_reason(unread_skb, SKB_DROP_REASON_UNIX_SKIP_OOB), so EPOLLPRI and
SIOCATMARK are cleared even though recv() reported 0 bytes.

Since these tests run with self->tcp_compliant == true and are not wrapped in
tcp_incompliant, do they implicitly claim TCP behaves the same here?  On the
TCP side a len == 0 receive leaves the urgent state alone:

net/ipv4/tcp.c:tcp_recvmsg_locked() {
	...
		if (unlikely(tp->urg_data)) {
			u32 urg_offset = tp->urg_seq - *seq;
			if (urg_offset < used) {
	...
}

with used == 0 the condition is false, so tp->urg_data and copied_seq stay
untouched and SIOCATMARK still returns 1 with EPOLLPRI set.  Adding
epollpair()/siocatmarkpair() after the new recvpair() calls would show
answ[0] == 0 versus answ[1] == 1, which looks like exactly the case the
tcp_incompliant idiom exists for.

For the peek fixture variant, recvpair() issues the MSG_PEEK receive first and
then the normal one, but with an expectation of 0 bytes is there anything left
that would catch a regression where the MSG_PEEK pass itself dropped or
consumed the queued OOB skb?

> +
> +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);
> +}

[Severity: Medium]
The core patch in this series enumerated three distinct cases that
manage_oob() gets wrong for a zero-length receive, but the new tests
only seem to cover two of them.  zero_buf_non_oob_oob() and
zero_buf_non_oob_oob_blocking() send "ab" with MSG_OOB and then do a
0-byte recv, which is case 1 (a normal skb followed by a live OOB skb,
u->oob_skb still set).  zero_buf_ex_oob_oob() and its _blocking twin
cover case 3 (an already consumed OOB skb followed by a new OOB skb).
I cannot find a test for case 2, where the queue head is a plain skb
followed by an OOB skb whose OOB byte has already been consumed, so
u->oob_skb no longer points at it and manage_oob() has to take the
skip-consumed-OOB path instead of the unlink path.

Would it be worth adding something like sendpair("ab", 2, MSG_OOB) then
recvpair("b", 1, 1, MSG_OOB) to consume the OOB byte while leaving "a"
and the consumed OOB skb queued, followed by recvpair("", 0, 0, 0)?
Without it, the commit message's claim that this patch adds tests
"corresponding" to the previous fixes does not quite match what is
here, and the case that led to the busy-wait/CPU hog in the consumed
skb path stays uncovered.  Or is case 2 already exercised indirectly by
one of the existing tests in a way I am missing?

[Severity: Medium]

The commit message says "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."  Do the new tests actually cover the second fix, the
last-skb marker updates from "af_unix: Update last skb marker in
manage_oob()"?

All six tests call recvpair() with buf_len == 0, including the three blocking
ones, so state->size == 0 and unix_stream_read_generic() leaves as soon as
manage_oob() returns NULL:

net/unix/af_unix.c:unix_stream_read_generic() {
	...
			skb = manage_oob(skb, &last, sk, flags, copied);
			if (!skb && (copied || !state->size)) {
				unix_state_unlock(sk);
				break;
			}
	...
}

With size == 0 the loop cannot iterate and copied never becomes non-zero, so
unix_stream_data_wait(sk, timeo, last, freezable) is never reached and the
*last assignments in manage_oob() are never observed.

Would all 12 new cases (6 tests x 2 fixture variants) still pass if those
*last updates were reverted?  Case 3 in that commit message uses a non-zero
length blocking MSG_PEEK:

	send "a" MSG_OOB
	recv(buf, 1, MSG_OOB)
	send "b" MSG_OOB
	recv(buf, 1, MSG_PEEK)

zero_buf_ex_oob_oob_blocking() uses recvpair("", 0, 0, 0) instead of a 1-byte
MSG_PEEK, so the busy-wait that fix addresses appears to be left without
coverage.  Would adding a blocking test with buf_len == 1 and MSG_PEEK close
that gap?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902202202.892676-1-kuniyu%40google.com