Re: [PATCH net v2 2/2] af_unix: skip consumed OOB skb and pull consumed bytes in unix_stream_read_skb()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] net/unix/af_unix.c, unix_stream_read_skb(): the newly added pull
  block frees the whole SKB and returns -ENOMEM when pskb_pull(skb,…

Pre-existing issues:
- [Medium] Unbalanced u->scm_stat.nr_fds accounting on AF_UNIX skb drop
  paths.

--

af_unix: skip consumed OOB skb and pull consumed bytes in unix_stream_read_skb()

This changes unix_stream_read_skb() to drop zero-length skbs left behind
after an OOB byte was consumed, and to pull UNIXCB(skb).consumed bytes off
a partially consumed skb before handing it to recv_actor().  Without this,
sockmap readers re-deliver bytes that a previous recv() already consumed.

> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 0903bd587696..a1c06b9995c7 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -2885,6 +2885,7 @@ static int unix_stream_read_skb(struct sock *sk, skb_read_actor_t recv_actor)
>  		return err;
>  
>  	mutex_lock(&u->iolock);
> +again:
>  	spin_lock(&queue->lock);
>  
>  	skb = __skb_dequeue(queue);
> @@ -2894,6 +2895,13 @@ static int unix_stream_read_skb(struct sock *sk, skb_read_actor_t recv_actor)
>  		return -EAGAIN;
>  	}
>  
> +	if (!unix_skb_len(skb)) {
> +		spin_unlock(&queue->lock);
> +		unix_orphan_scm(sk, skb);
> +		consume_skb(skb);
> +		goto again;
> +	}
> +

[Severity: Medium]
This isn't a bug introduced by this patch, but the new block here uses the
correct pairing (unix_orphan_scm() before consume_skb()) while several
neighbouring free paths for the same class of skb do not, so it highlights
an existing imbalance in u->scm_stat.nr_fds.

scm_stat_del(), reached only from unix_orphan_scm(), is the only place that
does the matching decrement:

net/unix/af_unix.c:scm_stat_del() {
	...
	if (unlikely(fp && fp->count)) {
		atomic_sub(fp->count, &u->scm_stat.nr_fds);
		unix_del_edges(fp);
	}
	...
}

unix_destruct_scm() -> unix_detach_fds() -> unix_destroy_fpl() releases the
files and the GC edges but never touches nr_fds.

The OOB drop branch a few lines below in this same function frees a skb that
may still carry UNIXCB(skb).fp without going through unix_orphan_scm():

	if (skb == u->oob_skb) {
		WRITE_ONCE(u->oob_skb, NULL);
		spin_unlock(&queue->lock);
		mutex_unlock(&u->iolock);

		kfree_skb_reason(skb, SKB_DROP_REASON_UNIX_SKIP_OOB);
		return -EAGAIN;
	}

manage_oob() does the same with a bare consume_skb(read_skb) /
kfree_skb_reason(unread_skb), and unix_stream_recv_urg() frees a zero-length
predecessor with consume_skb(read_skb).

unix_stream_recv_urg() is also what creates the zero-length-with-fds state:
it copies the OOB byte and bumps UNIXCB(oob_skb).consumed without detaching
fds, so the leftover skb can still hold SCM_RIGHTS charged earlier by
scm_stat_add() in queue_oob().

Should those paths also call unix_orphan_scm() so the nr_fds charge reported
as scm_fds: in /proc/<pid>/fdinfo does not stay permanently inflated?

>  	WRITE_ONCE(u->inq_len, u->inq_len - unix_skb_len(skb));
>  
>  #if IS_ENABLED(CONFIG_AF_UNIX_OOB)
> @@ -2913,6 +2921,14 @@ static int unix_stream_read_skb(struct sock *sk, skb_read_actor_t recv_actor)
>  
>  	mutex_unlock(&u->iolock);
>  
> +	if (UNIXCB(skb).consumed) {
> +		if (!pskb_pull(skb, UNIXCB(skb).consumed)) {
> +			kfree_skb(skb);
> +			return -ENOMEM;
> +		}
> +		UNIXCB(skb).consumed = 0;
> +	}
> +

[Severity: High]
Can this kfree_skb() discard unread stream bytes?

At this point in unix_stream_read_skb() the skb has already been unlinked by
__skb_dequeue(), and u->inq_len has already been reduced:

	WRITE_ONCE(u->inq_len, u->inq_len - unix_skb_len(skb));

so once the skb is freed the remaining unix_skb_len(skb) bytes are neither
recoverable nor still accounted for in the queue length.

The commit message describes pskb_pull() as the way to "safely handle both
linear and non-linear paged skbs", but it can still fail.  For a pull past
the linear head it goes pskb_pull() -> pskb_may_pull() ->
__pskb_pull_tail() -> pskb_expand_head():

net/core/skbuff.c:__pskb_pull_tail() {
	...
	if (eat > 0 || skb_cloned(skb)) {
		if (pskb_expand_head(skb, 0, eat > 0 ? eat + 128 : 0,
				     GFP_ATOMIC))
			return NULL;
	}
	...
}

unix_stream_sendmsg() produces exactly the skbs that reach that branch:

net/unix/af_unix.c:unix_stream_sendmsg() {
	...
		data_len = max_t(int, 0, size - SKB_MAX_HEAD(0));
		data_len = min_t(size_t, size, PAGE_ALIGN(data_len));
		skb = sock_alloc_send_pskb(sk, size - data_len, data_len, ...);
	...
}

MSG_SPLICE_PAGES skbs have skb_headlen() == 0 entirely, so any partial
recv() leaves UNIXCB(skb).consumed beyond the linear head and forces the
fallible path with a GFP_ATOMIC expansion of up to the full frag size.

The return value also does not appear to reach anyone who can retry:

net/core/skmsg.c:sk_psock_verdict_data_ready() {
	...
	copied = ops->read_skb(sk, sk_psock_verdict_recv);
	if (copied >= 0) {
		...
		sk_psock_data_ready(sk, psock);
	}
	...
}

The -ENOMEM is swallowed, sk_psock_data_ready() is skipped for data still
queued behind the dropped skb, and the sockmap consumer sees a hole in an
ordered byte stream with no error reported to userspace.

Would it be better to restore the state on failure, for example
__skb_queue_head() plus adding unix_skb_len(skb) back to u->inq_len, and let
the caller come back later, instead of freeing the skb?

>  	return recv_actor(sk, skb);
>  }
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921054823.1822122-1-benquike%40gmail.com