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

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

[mheib]

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: 3a1296a38d0c ("net: Support GRO/GSO fraglist chaining.")
Signed-off-by: Mohammad Heib <mheib@redhat.com>
---
 net/core/skbuff.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index a00808f7be6a..aee9be42409b 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4641,6 +4641,7 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
 	struct sk_buff *tail = NULL;
 	struct sk_buff *nskb, *tmp;
 	int len_diff, err;
+	bool is_flist = !!(skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST);
 
 	skb_push(skb, -skb_network_offset(skb) + offset);
 
@@ -4656,7 +4657,15 @@ 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 if the fragment is being orphaned.
+		 * In the GRO path, fragments don't have a socket owner (sk=NULL),
+		 * so the parent must maintain the total truesize to prevent
+		 * memory accounting leaks.
+		 */
+		if (!is_flist || nskb->sk)
+			delta_truesize += nskb->truesize;
+
 		if (skb_shared(nskb)) {
 			tmp = skb_clone(nskb, GFP_ATOMIC);
 			if (tmp) {
@@ -4684,7 +4693,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 (!is_flist || 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

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

[Willem de Bruijn]

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.

We have to separate packets that use frag_list, a broader category,
from those that use fraglist gso chaining. This code path is only
exercised by the latter.

> 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 was not present in the original fraglist chaining patch cited in
the Fixes tag. It was added in commit ed4cccef64c1 ("gro: fix
ownership transfer"). Which was a follow-on to commit 5e10da5385d2
("skbuff: allow 'slow_gro' for skb carring sock reference") removing
the skb->destructor reference.
 
> 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.

Thanks for the report and fix. It can probably be simplified a bit
based on knowledge that only fraglist chaining skbs reach this path.
And the Fixes tag should reflect the patch that changed this
accounting in the GRO patch. Matching that in the GSO path makes
sense.

> Fixes: 3a1296a38d0c ("net: Support GRO/GSO fraglist chaining.")
> Signed-off-by: Mohammad Heib <mheib@redhat.com>
> ---
>  net/core/skbuff.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index a00808f7be6a..aee9be42409b 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4641,6 +4641,7 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
>  	struct sk_buff *tail = NULL;
>  	struct sk_buff *nskb, *tmp;
>  	int len_diff, err;
> +	bool is_flist = !!(skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST);

This is guaranteed when entering skb_segment_list.

>  
>  	skb_push(skb, -skb_network_offset(skb) + offset);
>  
> @@ -4656,7 +4657,15 @@ 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 if the fragment is being orphaned.
> +		 * In the GRO path, fragments don't have a socket owner (sk=NULL),
> +		 * so the parent must maintain the total truesize to prevent
> +		 * memory accounting leaks.
> +		 */
> +		if (!is_flist || nskb->sk)
> +			delta_truesize += nskb->truesize;
> +
>  		if (skb_shared(nskb)) {
>  			tmp = skb_clone(nskb, GFP_ATOMIC);
>  			if (tmp) {
> @@ -4684,7 +4693,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 (!is_flist || 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
>