[PATCH net v2 0/2] af_unix: fix inq_len accounting and consumed OOB skb handling in unix_stream_read_skb()

[PATCH net v2 0/2] af_unix: fix inq_len accounting and consumed OOB skb handling in unix_stream_read_skb()

[Hui Peng]

This series addresses two OOB skb handling bugs in AF_UNIX stream sockets:

1. Decrement u->inq_len by 1 when manage_oob() unlinks an unread OOB skb,
   preventing u->inq_len from remaining permanently inflated and causing
   SIOCINQ / FIONREAD to report a stale positive byte count on an empty
   socket.
2. Skip zero-length consumed OOB skbs (after releasing SCM rights via
   unix_orphan_scm()) and pull UNIXCB(skb).consumed bytes via pskb_pull()
   in unix_stream_read_skb(), preventing BPF sockmap from re-delivering
   already consumed OOB bytes or partially consumed skb prefixes.

Changes in v2:
- Split into a 2-patch series as requested by Kuniyuki Iwashima.
- Update Fixes: tag to f4e1fb04c123 ("af_unix: Use cached value for
  SOCK_STREAM in unix_inq_len().").
- Call unix_orphan_scm(sk, skb) before consume_skb(skb) and use
  pskb_pull() instead of skb_pull() to safely handle non-linear paged skbs
  as noted by Sashiko.

Hui Peng (2):
  af_unix: decrement u->inq_len when skipping unread OOB skb in manage_oob()
  af_unix: skip consumed OOB skb and pull consumed bytes in unix_stream_read_skb()

 net/unix/af_unix.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

[PATCH net v2 1/2] af_unix: decrement u->inq_len when skipping unread OOB skb in manage_oob()

[Hui Peng]

When a normal in-band read on an AF_UNIX stream socket (without MSG_PEEK
and without SO_OOBINLINE) encounters an unread u->oob_skb in manage_oob(),
manage_oob() clears u->oob_skb, unlinks the 1-byte skb from
sk->sk_receive_queue, and drops it with SKB_DROP_REASON_UNIX_SKIP_OOB
without decrementing u->inq_len. Because queue_oob() incremented
u->inq_len by 1 when queuing the OOB skb, u->inq_len remains permanently
inflated by 1 byte for each skipped unread OOB skb, causing SIOCINQ /
FIONREAD to report a stale positive byte count on an empty socket.

Decrement u->inq_len by 1 when unlinking the unread OOB skb in
manage_oob().

Tested in QEMU against Linux 7.3.0-rc3 by sending three 1-byte MSG_OOB
packets interleaved with normal stream data on an AF_UNIX SOCK_STREAM
socketpair and draining all in-band data via recv(): on the unfixed
kernel, ioctl(SIOCINQ) reports 3 on the empty socket; with this patch
applied, ioctl(SIOCINQ) reports 0.

Fixes: f4e1fb04c123 ("af_unix: Use cached value for SOCK_STREAM in unix_inq_len().")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
Changes in v2:
- Split the manage_oob() and unix_stream_read_skb() fixes into a 2-patch
  series as requested by Kuniyuki Iwashima.
- Update Fixes: tag to f4e1fb04c123 ("af_unix: Use cached value for
  SOCK_STREAM in unix_inq_len().") and clarify the commit message as
  noted by Sashiko.

 net/unix/af_unix.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 42cffeafc8c1..1770af3c2684 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2849,6 +2849,7 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sk_buff **last,
 		WRITE_ONCE(u->oob_skb, NULL);
 
 		if (!sock_flag(sk, SOCK_URGINLINE)) {
+			WRITE_ONCE(u->inq_len, u->inq_len - 1);
 			__skb_unlink(skb, &sk->sk_receive_queue);
 			unread_skb = skb;
 			skb = skb_peek(&sk->sk_receive_queue);
-- 
2.55.0.1082.g2b9226bbc0-goog

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

[Hui Peng]

When an OOB byte is consumed via recv(MSG_OOB) in unix_stream_recv_urg(),
u->oob_skb is cleared to NULL and UNIXCB(oob_skb).consumed is incremented
to 1, but oob_skb remains on sk->sk_receive_queue (with unix_skb_len(skb)
== 0) to preserve the OOB mark until normal reads advance past it.
Similarly, a partial recv() in unix_stream_read_generic() advances
UNIXCB(skb).consumed without pulling the skb header and leaves the
partially consumed skb at the head of sk->sk_receive_queue.

If the socket is subsequently read via unix_stream_read_skb() (used by
BPF sockmap), unix_stream_read_skb() only checks skb == u->oob_skb
(which is only true for an unconsumed OOB skb) and ignores
UNIXCB(skb).consumed. As a result, a consumed OOB skb (unix_skb_len(skb)
== 0) is handed to recv_actor() and re-delivers the already consumed OOB
byte, and a partially consumed skb re-delivers its already consumed
prefix.

In unix_stream_read_skb(), skip and free zero-length consumed skbs (after
calling unix_orphan_scm() so SCM_RIGHTS fd accounting remains balanced)
and pull UNIXCB(skb).consumed bytes via pskb_pull() (which safely handles
both linear and non-linear paged skbs) before invoking recv_actor().

Tested in QEMU against Linux 7.3.0-rc3 with BPF_MAP_TYPE_SOCKMAP and a
BPF_SK_SKB_STREAM_VERDICT program:
1. Consuming a 1-byte MSG_OOB packet ("Z") before inserting the socket
   into sockmap and sending "HELLO" re-delivers "ZHELLO" on the unfixed
   kernel, whereas with this patch applied recv() receives "HELLO!".
2. Consuming a 3-byte prefix ("123") of "12345678" before inserting the
   socket into sockmap re-delivers "12345678" on the unfixed kernel,
   whereas with this patch applied recv() receives "45678".

Fixes: 77462de14a43 ("af_unix: Add read_sock for stream socket types")
Fixes: 314001f0bf92 ("af_unix: Add OOB support")
Fixes: 638f32604385 ("af_unix: Disable MSG_OOB handling for sockets in sockmap/sockhash")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
Changes in v2:
- Split out from the manage_oob() fix into patch 2/2 as requested by
  Kuniyuki Iwashima.
- Call unix_orphan_scm(sk, skb) before consume_skb(skb) when dropping a
  zero-length consumed skb so u->scm_stat.nr_fds is decremented, and use
  pskb_pull() instead of skb_pull() to safely handle non-linear paged
  skbs without hitting BUG() in __skb_pull(), as noted by Sashiko.

 net/unix/af_unix.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 1770af3c2684..f5d64e7b5c1c 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;
+	}
+
 	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;
+	}
+
 	return recv_actor(sk, skb);
 }
 
-- 
2.55.0.1082.g2b9226bbc0-goog

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 · 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] The patch adds a pskb_pull() call to unix_stream_read_skb()
  (net/unix/af_unix.c:2924-2930).

--

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

This patch makes unix_stream_read_skb() free zero-length consumed skbs
(for example an OOB skb already read with recv(MSG_OOB)). It also makes
it pull UNIXCB(skb).consumed bytes before handing the skb to
recv_actor(). The goal is to stop sockmap from re-delivering data that a
normal recv() has 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

[ ... ]

> @@ -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;
> +		}

[Severity: High]
If pskb_pull() fails here, is the unread part of the stream silently
lost?

unix_stream_sendmsg() often builds non-linear skbs. The normal path uses
sock_alloc_send_pskb(sk, size - data_len, data_len, ...), which caps the
linear head and puts the rest in frags. The MSG_SPLICE_PAGES path uses
sock_alloc_send_pskb(sk, 0, 0, ...), so headlen is 0.

After a partial recv(), unix_stream_read_generic() does:

    UNIXCB(skb).consumed += chunk;

and leaves the skb on the queue. That means consumed can be larger than
skb_headlen(skb), in which case pskb_pull() takes the allocating path:

    pskb_pull()
      pskb_may_pull_reason()
        __pskb_pull_tail(skb, consumed - headlen)
          pskb_expand_head(skb, 0, eat + 128, GFP_ATOMIC)

This is an atomic allocation, possibly high order, sized by the prefix
that was already consumed. On success, that prefix is copied out of the
frags only to be discarded.

If the allocation fails, kfree_skb(skb) frees the whole skb, including
the unix_skb_len(skb) bytes nobody has read yet. By then u->inq_len has
already been reduced:

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

The only caller, sk_psock_verdict_data_ready() in net/core/skmsg.c,
only acts on non-negative returns:

    copied = ops->read_skb(sk, sk_psock_verdict_recv);
    if (copied >= 0) {

So the -ENOMEM is dropped: nothing retries, sk_err is not set, and the
skb is already off sk_receive_queue. Before this patch, this spot had no
allocation-failure path.

The commit message says pskb_pull() "safely handles both linear and
non-linear paged skbs". That is true in that it avoids the BUG() in
__skb_pull(), but it can still fail and drop data.

Could the consumed bytes be skipped without allocating instead? Two
options: trim the head and frags in place, as TCP's __pskb_trim_head()
does, or pass an offset to the actor.

> +		UNIXCB(skb).consumed = 0;
> +	}
> +
>  	return recv_actor(sk, skb);
>  }

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