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

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.

[ ... ]

[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