Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <0b026eb24a0348d32b6dda94950f6517f8304456.camel@redhat.com>
Subject: Re: [PATCH] net:fix up skbs delta_truesize in UDP GRO frag_list
From:   Paolo Abeni <pabeni@redhat.com>
To:     Lena Wang <lena.wang@mediatek.com>, davem@davemloft.net,
        kuba@kernel.org, matthias.bgg@gmail.com
Cc:     wsd_upstream@mediatek.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, hao.lin@mediatek.com,
        Eric Dumazet <edumazet@google.com>
Date:   Fri, 25 Feb 2022 09:46:34 +0100
In-Reply-To: <1645769353-7171-2-git-send-email-lena.wang@mediatek.com>
References: <1645769353-7171-1-git-send-email-lena.wang@mediatek.com>
         <1645769353-7171-2-git-send-email-lena.wang@mediatek.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.3 (3.42.3-1.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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