[PATCH bpf] bpf, sockmap: fix page_counter underflow in strparser SK_PASS

[PATCH bpf] bpf, sockmap: fix page_counter underflow in strparser SK_PASS

[Junseo Lim]

tcp_bpf_strp_read_sock() delays cleanup of SK_PASS bytes by
subtracting psock->ingress_bytes from the amount passed to
__tcp_cleanup_rbuf(). But when sk_psock_verdict_apply() queues the skb
directly through sk_psock_skb_ingress_self(), skb_set_owner_r() is called
unconditionally and charges the skb again.

The duplicated charge is later released independently and can trigger a
page_counter underflow.

Add a charge_skb argument to sk_psock_skb_ingress_self() and skip
skb_set_owner_r() only for the direct strparser SK_PASS path. Keep existing
accounting for the other self-ingress caller and for non-strparser SK_PASS.

Fixes: 36b62df5683c ("bpf: Fix wrong copied_seq calculation")
Signed-off-by: Junseo Lim <zirajs7@gmail.com>
---
Crash reproduced on Linux tree 94515f3a7d4256a5062176b7d6ed0471938cd51a
with KASAN, MEMCG, panic_on_warn=1, and oops=panic.

Reproducer/log/config: https://gist.github.com/ZirAjs/16c95c89972ace73910d9b5ac78a5807

The reproducer drives the strparser SK_PASS path until teardown reports:

  page_counter underflow
  Workqueue: events sk_psock_destroy
  Kernel panic - not syncing: kernel: panic_on_warn set ...

 net/core/skmsg.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index 2521b643fa05..17260681479a 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -586,7 +586,8 @@ static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb,
 }
 
 static int sk_psock_skb_ingress_self(struct sk_psock *psock, struct sk_buff *skb,
-				     u32 off, u32 len, bool take_ref);
+				     u32 off, u32 len, bool take_ref,
+				     bool charge_skb);
 
 static int sk_psock_skb_ingress(struct sk_psock *psock, struct sk_buff *skb,
 				u32 off, u32 len)
@@ -595,12 +596,9 @@ static int sk_psock_skb_ingress(struct sk_psock *psock, struct sk_buff *skb,
 	struct sk_msg *msg;
 	int err;
 
-	/* If we are receiving on the same sock skb->sk is already assigned,
-	 * skip memory accounting and owner transition seeing it already set
-	 * correctly.
-	 */
 	if (unlikely(skb->sk == sk))
-		return sk_psock_skb_ingress_self(psock, skb, off, len, true);
+		return sk_psock_skb_ingress_self(psock, skb, off, len, true,
+						 true);
 	msg = sk_psock_create_ingress_msg(sk, skb);
 	if (!msg)
 		return -EAGAIN;
@@ -618,12 +616,14 @@ static int sk_psock_skb_ingress(struct sk_psock *psock, struct sk_buff *skb,
 	return err;
 }
 
-/* Puts an skb on the ingress queue of the socket already assigned to the
- * skb. In this case we do not need to check memory limits or skb_set_owner_r
- * because the skb is already accounted for here.
+/* Puts an skb on the ingress queue for psock->sk.
+ *
+ * When charge_skb is false, the direct strparser SK_PASS path keeps the TCP
+ * receive queue accounting in place and must not call skb_set_owner_r().
  */
 static int sk_psock_skb_ingress_self(struct sk_psock *psock, struct sk_buff *skb,
-				     u32 off, u32 len, bool take_ref)
+				     u32 off, u32 len, bool take_ref,
+				     bool charge_skb)
 {
 	struct sk_msg *msg = alloc_sk_msg(GFP_ATOMIC);
 	struct sock *sk = psock->sk;
@@ -631,7 +631,8 @@ static int sk_psock_skb_ingress_self(struct sk_psock *psock, struct sk_buff *skb
 
 	if (unlikely(!msg))
 		return -EAGAIN;
-	skb_set_owner_r(skb, sk);
+	if (charge_skb)
+		skb_set_owner_r(skb, sk);
 
 	/* This is used in tcp_bpf_recvmsg_parser() to determine whether the
 	 * data originates from the socket's own protocol stack. No need to
@@ -1017,6 +1018,8 @@ static int sk_psock_verdict_apply(struct sk_psock *psock, struct sk_buff *skb,
 		 * retrying later from workqueue.
 		 */
 		if (skb_queue_empty(&psock->ingress_skb)) {
+			bool charge_skb = true;
+
 			len = skb->len;
 			off = 0;
 			if (skb_bpf_strparser(skb)) {
@@ -1024,8 +1027,10 @@ static int sk_psock_verdict_apply(struct sk_psock *psock, struct sk_buff *skb,
 
 				off = stm->offset;
 				len = stm->full_len;
+				charge_skb = false;
 			}
-			err = sk_psock_skb_ingress_self(psock, skb, off, len, false);
+			err = sk_psock_skb_ingress_self(psock, skb, off, len,
+							false, charge_skb);
 		}
 		if (err < 0) {
 			spin_lock_bh(&psock->ingress_lock);
-- 
2.55.0

Re: [PATCH bpf] bpf, sockmap: fix page_counter underflow in strparser SK_PASS

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Bypassing memory accounting in strparser SK_PASS path allows remote memory exhaustion (DoS)
--

--- Patch [1]: [PATCH bpf] bpf, sockmap: fix page_counter underflow in strparser SK_PASS ---
commit 5073ca36b1b9677d098ec43e3f231a7423b5d727
Author: Junseo Lim <zirajs7@gmail.com>

bpf, sockmap: fix page_counter underflow in strparser SK_PASS
    
This patch adds a charge_skb flag to skip memory accounting in the direct
strparser SK_PASS path. It attempts to fix a page_counter underflow by
preventing duplicate charges on the cloned SKB.

> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index 2521b643fa05d..17260681479a3 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c

[ ... ]

> @@ -618,12 +616,14 @@ static int sk_psock_skb_ingress(struct sk_psock *psock, struct sk_buff *skb,

[ ... ]

>  static int sk_psock_skb_ingress_self(struct sk_psock *psock, struct sk_buff *skb,
> -				     u32 off, u32 len, bool take_ref)
> +				     u32 off, u32 len, bool take_ref,
> +				     bool charge_skb)
>  {
>  	struct sk_msg *msg = alloc_sk_msg(GFP_ATOMIC);
>  	struct sock *sk = psock->sk;
> @@ -631,7 +631,8 @@ static int sk_psock_skb_ingress_self(struct sk_psock *psock, struct sk_buff *skb
>  
>  	if (unlikely(!msg))
>  		return -EAGAIN;
> -	skb_set_owner_r(skb, sk);
> +	if (charge_skb)
> +		skb_set_owner_r(skb, sk);

[Severity: Critical]
Does skipping skb_set_owner_r() here leave the cloned SKB without an
accounted truesize and without a destructor?

When charge_skb is false, the cloned SKB is queued without being charged.
At the same time, __tcp_read_sock() unlinks and frees the original SKB via
tcp_eat_recv_skb().

This invokes sock_rfree() and immediately subtracts skb->truesize from
sk->sk_rmem_alloc.

Because this queued clone lacks a destructor and was never charged, could
sk_rmem_alloc artificially drop to 0?

If so, tcp_select_window() might compute free_space as if the buffer is
entirely empty, continuously advertising a fully open TCP window and
potentially allowing a remote attacker to stream infinite data and exhaust
kernel memory.

[ ... ]

> @@ -1017,6 +1018,8 @@ static int sk_psock_verdict_apply(struct sk_psock *psock, struct sk_buff *skb,
>  		 * retrying later from workqueue.
>  		 */
>  		if (skb_queue_empty(&psock->ingress_skb)) {
> +			bool charge_skb = true;
> +
>  			len = skb->len;
>  			off = 0;
>  			if (skb_bpf_strparser(skb)) {
> @@ -1024,8 +1027,10 @@ static int sk_psock_verdict_apply(struct sk_psock *psock, struct sk_buff *skb,
>  
>  				off = stm->offset;
>  				len = stm->full_len;
> +				charge_skb = false;
>  			}
> -			err = sk_psock_skb_ingress_self(psock, skb, off, len, false);
> +			err = sk_psock_skb_ingress_self(psock, skb, off, len,
> +							false, charge_skb);
>  		}
>  		if (err < 0) {
>  			spin_lock_bh(&psock->ingress_lock);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723065244.186916-1-zirajs7@gmail.com?part=1

Re: [PATCH bpf] bpf, sockmap: fix page_counter underflow in strparser SK_PASS

[Emil Tsalapatis]

On Thu Jul 23, 2026 at 2:52 AM EDT, Junseo Lim wrote:
> tcp_bpf_strp_read_sock() delays cleanup of SK_PASS bytes by
> subtracting psock->ingress_bytes from the amount passed to
> __tcp_cleanup_rbuf(). But when sk_psock_verdict_apply() queues the skb
> directly through sk_psock_skb_ingress_self(), skb_set_owner_r() is called
> unconditionally and charges the skb again.

Can you specify in the commit that this is about sk_forward_alloc (AFAICT)?
The eventual page_counter underflow is a second-order effect even if it's
what eventually causes the crash.

>
> The duplicated charge is later released independently and can trigger a
> page_counter underflow.
>
> Add a charge_skb argument to sk_psock_skb_ingress_self() and skip
> skb_set_owner_r() only for the direct strparser SK_PASS path. Keep existing
> accounting for the other self-ingress caller and for non-strparser SK_PASS.
>
> Fixes: 36b62df5683c ("bpf: Fix wrong copied_seq calculation")
> Signed-off-by: Junseo Lim <zirajs7@gmail.com>
> ---
> Crash reproduced on Linux tree 94515f3a7d4256a5062176b7d6ed0471938cd51a
> with KASAN, MEMCG, panic_on_warn=1, and oops=panic.

Can you make a selftest out of the reproducer?

>
> Reproducer/log/config: https://gist.github.com/ZirAjs/16c95c89972ace73910d9b5ac78a5807
>
> The reproducer drives the strparser SK_PASS path until teardown reports:
>
>   page_counter underflow
>   Workqueue: events sk_psock_destroy
>   Kernel panic - not syncing: kernel: panic_on_warn set ...
>
>  net/core/skmsg.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index 2521b643fa05..17260681479a 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -586,7 +586,8 @@ static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb,
>  }
>  
>  static int sk_psock_skb_ingress_self(struct sk_psock *psock, struct sk_buff *skb,
> -				     u32 off, u32 len, bool take_ref);
> +				     u32 off, u32 len, bool take_ref,
> +				     bool charge_skb);
>  
>  static int sk_psock_skb_ingress(struct sk_psock *psock, struct sk_buff *skb,
>  				u32 off, u32 len)
> @@ -595,12 +596,9 @@ static int sk_psock_skb_ingress(struct sk_psock *psock, struct sk_buff *skb,
>  	struct sk_msg *msg;
>  	int err;
>  
> -	/* If we are receiving on the same sock skb->sk is already assigned,
> -	 * skip memory accounting and owner transition seeing it already set
> -	 * correctly.
> -	 */
>  	if (unlikely(skb->sk == sk))
> -		return sk_psock_skb_ingress_self(psock, skb, off, len, true);
> +		return sk_psock_skb_ingress_self(psock, skb, off, len, true,
> +						 true);
>  	msg = sk_psock_create_ingress_msg(sk, skb);
>  	if (!msg)
>  		return -EAGAIN;
> @@ -618,12 +616,14 @@ static int sk_psock_skb_ingress(struct sk_psock *psock, struct sk_buff *skb,
>  	return err;
>  }
>  
> -/* Puts an skb on the ingress queue of the socket already assigned to the
> - * skb. In this case we do not need to check memory limits or skb_set_owner_r
> - * because the skb is already accounted for here.
> +/* Puts an skb on the ingress queue for psock->sk.
> + *
> + * When charge_skb is false, the direct strparser SK_PASS path keeps the TCP
> + * receive queue accounting in place and must not call skb_set_owner_r().
>   */
>  static int sk_psock_skb_ingress_self(struct sk_psock *psock, struct sk_buff *skb,
> -				     u32 off, u32 len, bool take_ref)
> +				     u32 off, u32 len, bool take_ref,
> +				     bool charge_skb)
>  {
>  	struct sk_msg *msg = alloc_sk_msg(GFP_ATOMIC);
>  	struct sock *sk = psock->sk;
> @@ -631,7 +631,8 @@ static int sk_psock_skb_ingress_self(struct sk_psock *psock, struct sk_buff *skb
>  
>  	if (unlikely(!msg))
>  		return -EAGAIN;
> -	skb_set_owner_r(skb, sk);
> +	if (charge_skb)
> +		skb_set_owner_r(skb, sk);
Sashiko's right that skipping this leaves the skb half-configured. Since
the point is to not double-count sk_forward_alloc but we still have
to account truesize, can you try something like:

if (!skb_charge)
	sk_rmem_schedule(sk, skb, 0);

skb_set_owner_r(skb, sk);

to just ajust the sk_forward_alloc?

pw-bot: cr
>  
>  	/* This is used in tcp_bpf_recvmsg_parser() to determine whether the
>  	 * data originates from the socket's own protocol stack. No need to
> @@ -1017,6 +1018,8 @@ static int sk_psock_verdict_apply(struct sk_psock *psock, struct sk_buff *skb,
>  		 * retrying later from workqueue.
>  		 */
>  		if (skb_queue_empty(&psock->ingress_skb)) {
> +			bool charge_skb = true;
> +
>  			len = skb->len;
>  			off = 0;
>  			if (skb_bpf_strparser(skb)) {
> @@ -1024,8 +1027,10 @@ static int sk_psock_verdict_apply(struct sk_psock *psock, struct sk_buff *skb,
>  
>  				off = stm->offset;
>  				len = stm->full_len;
> +				charge_skb = false;
>  			}
> -			err = sk_psock_skb_ingress_self(psock, skb, off, len, false);
> +			err = sk_psock_skb_ingress_self(psock, skb, off, len,
> +							false, charge_skb);
>  		}
>  		if (err < 0) {
>  			spin_lock_bh(&psock->ingress_lock);