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

[mohammad heib <mheib@redhat.com>]

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.

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?

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