Re: [PATCH net v2] net: skbuff: fix truesize and head state corruption in skb_segment_list

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

mohammad heib wrote:
> Hi Willem,
> 
> You're right. I did a deeper dive into the callers and where the 
> SKB_GSO_FRAGLIST bit actually originates.
> 
> It turns out it is exclusively set in the GRO complete paths (tcp4, 
> udp4, tcp6, udp6). Since these packets are built by the GRO engine for 
> forwarding, the fragments are guaranteed to be orphans without socket 
> ownership or head state.

When revising, please include my short note on relevant prior patches.

Also, let's self document these known invariants with

    /* Only skb_gro_receive_list generated skbs arrive here */
    DEBUG_NET_WARN_ON_ONCE(!skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST);
    DEBUG_NET_WARN_ON_ONCE(nskb->sk);
 
> i will simply removed the truesize accumulation, as they are 
> inapplicable to this GSO type.
> 
> 
> One thing that’s confusing me is whether I should remove the call to 
> skb_release_head_state().
> This function updates reference counts for some fields in the skb, even 
> when no socket is attached to it.
> 
> So I’m wondering should I remove this call, or keep it as is?
> What do you think?

This was apparently introduced in commit cf673ed0e057 ("net: fix
fraglist segmentation reference count leak"). To handle a special
case for skbs with skb extensions. From that commit:

    "where the frag skbs
     will get the header copied from the head skb in skb_segment_list()
     by calling __copy_skb_header(), which could overwrite the frag skbs'
     extensions by __skb_ext_copy() and cause a leak."

So I read that has (1) this is still needed for some skbs (though the
only thing that is really needed is the skb_ext_reset(skb) call) and
(2) it is evidently safe to call unconditionally in skb_segment_list.

Same as previous, please include this context in the commit message.

> 
> On 12/31/25 6:58 PM, Willem de Bruijn wrote:
> > mheib@ wrote:
> >> From: Mohammad Heib <mheib@redhat.com>
> >>
> >> When skb_segment_list is called during packet forwarding through
> >> a bridge or VXLAN, it assumes that every fragment in a frag_list
> >> carries its own socket ownership and head state. While this is true for
> >> GSO packets created by the transmit path (via __ip_append_data), it is
> >> not true for packets built by the GRO receive path.
> >>
> >> In the GRO path, fragments are "orphans" (skb->sk == NULL) and were
> >> never charged to a socket. However, the current logic in
> >> skb_segment_list unconditionally adds every fragment's truesize to
> >> delta_truesize and subsequently subtracts this from the parent SKB.
> >>
> >> This results a memory accounting leak, Since GRO fragments were never
> >> charged to the socket in the first place, the "refund" results in the
> >> parent SKB returning less memory than originally charged when it is
> >> finally freed. This leads to a permanent leak in sk_wmem_alloc, which
> >> prevents the socket from being destroyed, resulting in a persistent memory
> >> leak of the socket object and its related metadata.
> >>
> >> The leak can be observed via KMEMLEAK when tearing down the networking
> >> environment:
> >>
> >> unreferenced object 0xffff8881e6eb9100 (size 2048):
> >>    comm "ping", pid 6720, jiffies 4295492526
> >>    backtrace:
> >>      kmem_cache_alloc_noprof+0x5c6/0x800
> >>      sk_prot_alloc+0x5b/0x220
> >>      sk_alloc+0x35/0xa00
> >>      inet6_create.part.0+0x303/0x10d0
> >>      __sock_create+0x248/0x640
> >>      __sys_socket+0x11b/0x1d0
> >>
> >> This patch modifies skb_segment_list to only perform head state release
> >> and truesize subtraction if the fragment explicitly owns a socket
> >> reference. For GRO-forwarded packets where fragments are not owners,
> >> the parent maintains the full truesize and acts as the single anchor for
> >> the memory refund upon destruction.
> >>
> >> Fixes: ed4cccef64c1 ("gro: fix ownership transfer")
> >> Signed-off-by: Mohammad Heib <mheib@redhat.com>
> >> ---
> >>   net/core/skbuff.c | 16 ++++++++++++++--
> >>   1 file changed, 14 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> >> index a00808f7be6a..63d3d76162ef 100644
> >> --- a/net/core/skbuff.c
> >> +++ b/net/core/skbuff.c
> >> @@ -4656,7 +4656,14 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
> >>   		list_skb = list_skb->next;
> >>   
> >>   		err = 0;
> >> -		delta_truesize += nskb->truesize;
> >> +
> >> +		/* Only track truesize delta and release head state for fragments
> >> +		 * that own a socket. GRO-forwarded fragments (sk == NULL) rely on
> >> +		 * the parent SKB for memory accounting.
> >> +		 */
> >> +		if (nskb->sk)
> >> +			delta_truesize += nskb->truesize;
> >> +
> > 
> > Similar to the previous point: if all paths that generate GSO packets
> > with SKB_GSO_FRAGLIST are generated from skb_gro_receive_list and that
> > function always sets skb->sk = NULL, is there even a need for this
> > brancy (and comment)?
> > 
> >>   		if (skb_shared(nskb)) {
> >>   			tmp = skb_clone(nskb, GFP_ATOMIC);
> >>   			if (tmp) {
> >> @@ -4684,7 +4691,12 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
> >>   
> >>   		skb_push(nskb, -skb_network_offset(nskb) + offset);
> >>   
> >> -		skb_release_head_state(nskb);
> >> +		/* For GRO-forwarded packets, fragments have no head state
> >> +		 * (no sk/destructor) to release. Skip this.
> >> +		 */
> >> +		if (nskb->sk)
> >> +			skb_release_head_state(nskb);
> >> +
> >>   		len_diff = skb_network_header_len(nskb) - skb_network_header_len(skb);
> >>   		__copy_skb_header(nskb, skb);
> >>   
> >> -- 
> >> 2.52.0
> >>
> > 
> > 
>