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