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

[netdev-bot+sashiko@kernel.org]

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