Re: [PATCH] net:fix up skbs delta_truesize in UDP GRO frag_list

Re: [PATCH] net:fix up skbs delta_truesize in UDP GRO frag_list

[Jakub Kicinski]

On Wed, 23 Feb 2022 18:08:47 +0800 lena.wang wrote:
> The truesize for a UDP GRO packet is added by main skb and skbs in main
> skb's frag_list:
> skb_gro_receive_list
>         p->truesize += skb->truesize;
> 
> When uncloning skb, it will call pskb_expand_head and trusesize for
> frag_list skbs may increase. This can occur when allocators uses
> __netdev_alloc_skb and not jump into __alloc_skb. This flow does not
> use ksize(len) to calculate truesize while pskb_expand_head uses.
> skb_segment_list
> err = skb_unclone(nskb, GFP_ATOMIC);
> pskb_expand_head
>         if (!skb->sk || skb->destructor == sock_edemux)
>                 skb->truesize += size - osize;
> 
> If we uses increased truesize adding as delta_truesize, it will be
> larger than before and even larger than previous total truesize value
> if skbs in frag_list are abundant. The main skb truesize will become
> smaller and even a minus value or a huge value for an unsigned int
> parameter. Then the following memory check will drop this abnormal skb.
> 
> To avoid this error we should use the original truesize to segment the
> main skb.
> 
> Signed-off-by: lena wang <lena.wang@mediatek.com>

CC: Eric

Re: [PATCH] net:fix up skbs delta_truesize in UDP GRO frag_list

[Jakub Kicinski]

On Thu, 24 Feb 2022 09:08:35 -0800 Jakub Kicinski wrote:
> On Wed, 23 Feb 2022 18:08:47 +0800 lena.wang wrote:
> > The truesize for a UDP GRO packet is added by main skb and skbs in main
> > skb's frag_list:
> > skb_gro_receive_list
> >         p->truesize += skb->truesize;
> > 
> > When uncloning skb, it will call pskb_expand_head and trusesize for
> > frag_list skbs may increase. This can occur when allocators uses
> > __netdev_alloc_skb and not jump into __alloc_skb. This flow does not
> > use ksize(len) to calculate truesize while pskb_expand_head uses.
> > skb_segment_list
> > err = skb_unclone(nskb, GFP_ATOMIC);
> > pskb_expand_head
> >         if (!skb->sk || skb->destructor == sock_edemux)
> >                 skb->truesize += size - osize;
> > 
> > If we uses increased truesize adding as delta_truesize, it will be
> > larger than before and even larger than previous total truesize value
> > if skbs in frag_list are abundant. The main skb truesize will become
> > smaller and even a minus value or a huge value for an unsigned int
> > parameter. Then the following memory check will drop this abnormal skb.
> > 
> > To avoid this error we should use the original truesize to segment the
> > main skb.
> > 
> > Signed-off-by: lena wang <lena.wang@mediatek.com>  

Eric pointed out this patch did not make it to the mailing list.
It was also whitespace damaged and line wrapped.

Could you resend with git send-email?

Re: [PATCH] net:fix up skbs delta_truesize in UDP GRO frag_list

[Jakub Kicinski]

On Fri, 25 Feb 2022 09:46:34 +0100 Paolo Abeni wrote:
> I *think* posting a v2 could be the easier way to handle the above
> glitches. If you do so (no changes to the patch body), please retain my
> ack.

Yup, please send a v2.

Re: [PATCH] net:fix up skbs delta_truesize in UDP GRO frag_list

[Paolo Abeni]

Hello,

Re-adding Eric, as Jakub cc-ed him in the previous iteration, but his
address has been lost here.

On Fri, 2022-02-25 at 14:09 +0800, Lena Wang wrote:
> From: lena wang <lena.wang@mediatek.com>
> 
> The truesize for a UDP GRO packet is added by main skb and skbs in main
> skb's frag_list:
> skb_gro_receive_list
>         p->truesize += skb->truesize;
> 
> When uncloning skb, it will call pskb_expand_head and trusesize for
> frag_list skbs may increase. This can occur when allocators uses
> __netdev_alloc_skb and not jump into __alloc_skb. This flow does not
> use ksize(len) to calculate truesize while pskb_expand_head uses.
> skb_segment_list
> err = skb_unclone(nskb, GFP_ATOMIC);
> pskb_expand_head
>         if (!skb->sk || skb->destructor == sock_edemux)
>                 skb->truesize += size - osize;
> 
> If we uses increased truesize adding as delta_truesize, it will be
> larger than before and even larger than previous total truesize value
> if skbs in frag_list are abundant. The main skb truesize will become
> smaller and even a minus value or a huge value for an unsigned int
> parameter. Then the following memory check will drop this abnormal skb.
> 
> To avoid this error we should use the original truesize to segment the
> main skb.

For some reasons 3 different mails with this patch lanted on the ML, I
guess 1 would have sufficed :)

This looks like a fix for the -net tree, please specify the target tree
in the patch subj and a suitable 'fixes' tag. Likely:

Fixes: 53475c5dd856 ("net: fix use-after-free when UDP GRO with shared fraglist")

> 
> 
> Signed-off-by: lena wang <lena.wang@mediatek.com>
> ---
>  net/core/skbuff.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 9d0388be..8b7356c 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3876,6 +3876,7 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
>  		list_skb = list_skb->next;
>  
>  		err = 0;
> +		delta_truesize += nskb->truesize;
>  		if (skb_shared(nskb)) {
>  			tmp = skb_clone(nskb, GFP_ATOMIC);
>  			if (tmp) {
> @@ -3900,7 +3901,6 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
>  		tail = nskb;
>  
>  		delta_len += nskb->len;
> -		delta_truesize += nskb->truesize;
>  
>  		skb_push(nskb, -skb_network_offset(nskb) + offset);
>  

Looks correct to me:

Acked-by: Paolo Abeni <pabeni@redhat.com>

I *think* posting a v2 could be the easier way to handle the above
glitches. If you do so (no changes to the patch body), please retain my
ack.

Cheers,

Paolo

[PATCH] net:fix up skbs delta_truesize in UDP GRO frag_list

[Lena Wang]

From: lena wang <lena.wang@mediatek.com>

The truesize for a UDP GRO packet is added by main skb and skbs in main
skb's frag_list:
skb_gro_receive_list
        p->truesize += skb->truesize;

When uncloning skb, it will call pskb_expand_head and trusesize for
frag_list skbs may increase. This can occur when allocators uses
__netdev_alloc_skb and not jump into __alloc_skb. This flow does not
use ksize(len) to calculate truesize while pskb_expand_head uses.
skb_segment_list
err = skb_unclone(nskb, GFP_ATOMIC);
pskb_expand_head
        if (!skb->sk || skb->destructor == sock_edemux)
                skb->truesize += size - osize;

If we uses increased truesize adding as delta_truesize, it will be
larger than before and even larger than previous total truesize value
if skbs in frag_list are abundant. The main skb truesize will become
smaller and even a minus value or a huge value for an unsigned int
parameter. Then the following memory check will drop this abnormal skb.

To avoid this error we should use the original truesize to segment the
main skb.

Signed-off-by: lena wang <lena.wang@mediatek.com>
---
 net/core/skbuff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 9d0388be..8b7356c 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3876,6 +3876,7 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
 		list_skb = list_skb->next;
 
 		err = 0;
+		delta_truesize += nskb->truesize;
 		if (skb_shared(nskb)) {
 			tmp = skb_clone(nskb, GFP_ATOMIC);
 			if (tmp) {
@@ -3900,7 +3901,6 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
 		tail = nskb;
 
 		delta_len += nskb->len;
-		delta_truesize += nskb->truesize;
 
 		skb_push(nskb, -skb_network_offset(nskb) + offset);
 
-- 
1.9.1

[PATCH] net:fix up skbs delta_truesize in UDP GRO frag_list

[Lena Wang]

From: lena wang <lena.wang@mediatek.com>

The truesize for a UDP GRO packet is added by main skb and skbs in main
skb's frag_list:
skb_gro_receive_list
        p->truesize += skb->truesize;

When uncloning skb, it will call pskb_expand_head and trusesize for
frag_list skbs may increase. This can occur when allocators uses
__netdev_alloc_skb and not jump into __alloc_skb. This flow does not
use ksize(len) to calculate truesize while pskb_expand_head uses.
skb_segment_list
err = skb_unclone(nskb, GFP_ATOMIC);
pskb_expand_head
        if (!skb->sk || skb->destructor == sock_edemux)
                skb->truesize += size - osize;

If we uses increased truesize adding as delta_truesize, it will be
larger than before and even larger than previous total truesize value
if skbs in frag_list are abundant. The main skb truesize will become
smaller and even a minus value or a huge value for an unsigned int
parameter. Then the following memory check will drop this abnormal skb.

To avoid this error we should use the original truesize to segment the
main skb.

Signed-off-by: lena wang <lena.wang@mediatek.com>
---
 net/core/skbuff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 9d0388be..8b7356c 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3876,6 +3876,7 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
 		list_skb = list_skb->next;
 
 		err = 0;
+		delta_truesize += nskb->truesize;
 		if (skb_shared(nskb)) {
 			tmp = skb_clone(nskb, GFP_ATOMIC);
 			if (tmp) {
@@ -3900,7 +3901,6 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
 		tail = nskb;
 
 		delta_len += nskb->len;
-		delta_truesize += nskb->truesize;
 
 		skb_push(nskb, -skb_network_offset(nskb) + offset);
 
-- 
1.9.1

[PATCH] net:fix up skbs delta_truesize in UDP GRO frag_list

[Lena Wang]

From: lena wang <lena.wang@mediatek.com>

The truesize for a UDP GRO packet is added by main skb and skbs in main
skb's frag_list:
skb_gro_receive_list
        p->truesize += skb->truesize;

When uncloning skb, it will call pskb_expand_head and trusesize for
frag_list skbs may increase. This can occur when allocators uses
__netdev_alloc_skb and not jump into __alloc_skb. This flow does not
use ksize(len) to calculate truesize while pskb_expand_head uses.
skb_segment_list
err = skb_unclone(nskb, GFP_ATOMIC);
pskb_expand_head
        if (!skb->sk || skb->destructor == sock_edemux)
                skb->truesize += size - osize;

If we uses increased truesize adding as delta_truesize, it will be
larger than before and even larger than previous total truesize value
if skbs in frag_list are abundant. The main skb truesize will become
smaller and even a minus value or a huge value for an unsigned int
parameter. Then the following memory check will drop this abnormal skb.

To avoid this error we should use the original truesize to segment the
main skb.

Signed-off-by: lena wang <lena.wang@mediatek.com>
---
 net/core/skbuff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 9d0388be..8b7356c 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3876,6 +3876,7 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
 		list_skb = list_skb->next;
 
 		err = 0;
+		delta_truesize += nskb->truesize;
 		if (skb_shared(nskb)) {
 			tmp = skb_clone(nskb, GFP_ATOMIC);
 			if (tmp) {
@@ -3900,7 +3901,6 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
 		tail = nskb;
 
 		delta_len += nskb->len;
-		delta_truesize += nskb->truesize;
 
 		skb_push(nskb, -skb_network_offset(nskb) + offset);
 
-- 
1.9.1