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

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.

[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