The strparser SK_PASS path can queue cloned skbs back to the same socket. A single TCP receive skb may be split into multiple strparser messages, and each cloned message still carries the receive owner from the TCP receive path. sk_psock_skb_ingress_self() reassigns receive ownership with skb_set_owner_r(). That first orphans the skb, which runs the existing receive destructor, and then charges the skb to the socket again. When this is repeated for strparser clones, sk_forward_alloc can already be in deficit before the next owner transition. Releasing the queued skbs can then uncharge more memcg pages than were reserved and trigger a page_counter underflow. Call sk_rmem_schedule() with a size of zero before skb_set_owner_r() for strparser self-pass skbs. Use the zero-sized reservation to top up any existing sk_forward_alloc deficit without reserving the skb's full truesize again, then let skb_set_owner_r() perform the receive-owner transition. Apply the same handling when retrying the skb from the psock backlog. Fixes: 144748eb0c44 ("bpf, sockmap: Fix incorrect fwd_alloc accounting") Reported-by: Sechang Lim Suggested-by: Emil Tsalapatis Signed-off-by: Junseo Lim --- net/core/skmsg.c | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/net/core/skmsg.c b/net/core/skmsg.c index 2521b643fa05..ce5ad8160282 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 settle_fwd_alloc); 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, + skb_bpf_strparser(skb)); 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; + } + skb_set_owner_r(skb, sk); /* This is used in tcp_bpf_recvmsg_parser() to determine whether the @@ -1017,6 +1024,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 settle_fwd_alloc = false; + len = skb->len; off = 0; if (skb_bpf_strparser(skb)) { @@ -1024,8 +1033,10 @@ static int sk_psock_verdict_apply(struct sk_psock *psock, struct sk_buff *skb, off = stm->offset; len = stm->full_len; + settle_fwd_alloc = true; } - err = sk_psock_skb_ingress_self(psock, skb, off, len, false); + err = sk_psock_skb_ingress_self(psock, skb, off, len, + false, settle_fwd_alloc); } if (err < 0) { spin_lock_bh(&psock->ingress_lock); -- 2.55.0