Re: [PATCH bpf v2 1/2] bpf, sockmap: settle sk_forward_alloc for strparser SK_PASS

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

New issues:
- [High] The sk_forward_alloc deficit fix is bypassed on the backlog retry path for strparser clones, leaving the memcg page_counter underflow bug unfixed and introducing a truesize double-charge.

Pre-existing issues:
- [High] The exact same page_counter underflow vulnerability exists for non-strparser SKBs that already have an owner, but the patch restricts the fix only to strparser SKBs.
- [High] Modification of socket memory accounting (sk_forward_alloc) without holding the socket lock in the psock backlog workqueue.
--

--- Patch [1]: [PATCH bpf v2 1/2] bpf, sockmap: settle sk_forward_alloc for strparser SK_PASS ---
commit 1a3d6fb5186f9d9fdd026f8cf86dcfc1debee455
Author: Junseo Lim <[email protected]>

bpf, sockmap: settle sk_forward_alloc for strparser SK_PASS

This patch attempts to fix a memcg page_counter underflow in the strparser
SK_PASS path. It adds a zero-sized sk_rmem_schedule() top-up before
reassigning receive ownership to direct strparser clones.

> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index 2521b643fa05d..ce5ad81602828 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
[ ... ]
> @@ -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,
> +						 skb_bpf_strparser(skb));

[Severity: High]
Does the backlog retry path bypass this fix for strparser clones?

If a strparser clone fails in the fast path and is queued to the backlog,
the workqueue retries it. However, before the retry occurs:

    sk_psock_backlog() {
        ...
        ingress = skb_bpf_ingress(skb);
        skb_bpf_redirect_clear(skb);
        ...
    }

The call to skb_bpf_redirect_clear() unconditionally erases the
BPF_F_STRPARSER flag. When the retry reaches sk_psock_skb_ingress(),
skb_bpf_strparser(skb) will evaluate to false, passing settle_fwd_alloc = false
and bypassing the fix.

>  	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.
> + *
> + * Before assigning receive ownership to a direct strparser SK_PASS clone,
> + * settle any existing sk_forward_alloc deficit from earlier clone charges.
>   */
>  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 settle_fwd_alloc)
>  {
>  	struct sk_msg *msg = alloc_sk_msg(GFP_ATOMIC);
>  	struct sock *sk = psock->sk;
> @@ -631,6 +631,13 @@ static int sk_psock_skb_ingress_self(struct sk_psock *psock, struct sk_buff *skb
>  
>  	if (unlikely(!msg))
>  		return -EAGAIN;
> +
> +	if (settle_fwd_alloc &&
> +	    !sk_rmem_schedule(sk, skb, 0)) {
> +		kfree(msg);
> +		return -EAGAIN;
> +	}
> +

[Severity: High]
If the fast path fails here at sk_rmem_schedule(), does the early return
bypass skb_set_owner_r() and set up a double-charge?

By returning -EAGAIN, skb->sk remains NULL. When the skb is retried from
the backlog, sk_psock_skb_ingress() will see skb->sk != sk. This bypasses
sk_psock_skb_ingress_self() entirely, falling back to charging the full
skb->truesize again instead of 0.

Also, this is a pre-existing issue, but does modifying socket memory
accounting here in the backlog workqueue cause a data race?

The variable sk->sk_forward_alloc is updated non-atomically via functions
like sk_mem_schedule(), which require lock_sock(sk) or bh_lock_sock(sk)
to prevent lost updates. When called from the backlog workqueue
(sk_psock_backlog()), deferred ingress packets are processed with only
psock->work_mutex held:

    sk_psock_backlog() {
        mutex_lock(&psock->work_mutex);
        while ((skb = skb_peek(&psock->ingress_skb))) {
            ...
            ret = sk_psock_handle_skb(psock, skb, off, len, ingress);
            ...
    }

Concurrent TCP softirq RX or process-context TX operations could race
with sk_rmem_schedule() here, potentially corrupting sk_forward_alloc.

>  	skb_set_owner_r(skb, sk);

[Severity: High]
This isn't a bug introduced by this patch, but does the same page_counter
underflow vulnerability exist for normal TCP SKBs that already have an owner?

The patch restricts the fix (settle_fwd_alloc = true) only to SKBs with the
BPF_F_STRPARSER flag. Normal TCP SKBs passed via sk_psock_verdict_recv()
bypass the sk_rmem_schedule() top-up. 

Because they already have skb->sk == sk and skb->destructor == sock_rfree,
they undergo the exact same uncharge-recharge cycle when skb_set_owner_r()
is unconditionally called here. Could they trigger the exact same underflow?

>  
>  	/* This is used in tcp_bpf_recvmsg_parser() to determine whether the
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1