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

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.

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

[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