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

["Emil Tsalapatis" <emil@etsalapatis.com>]

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);