af_unix: Minor fixes for MSG_OOB and MSG_PEEK.

net/unix/af_unix.c static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk, · line 2856

[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?

net/unix/af_unix.c static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk, · line 2861

[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?

net/unix/af_unix.c static int unix_stream_read_generic(struct unix_stream_read_state *state, · line 2981

[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

net/unix/af_unix.c static int unix_stream_read_generic(struct unix_stream_read_state *state, · line 2979

[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

tools/testing/selftests/net/af_unix/msg_oob.c TEST_F(msg_oob, inline_ex_oob_siocatmark) · line 917

[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?

tools/testing/selftests/net/af_unix/msg_oob.c TEST_F(msg_oob, inline_ex_oob_siocatmark) · line 956

[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