[PATCH net-next] net: fix up truesize of cloned skb in skb_prepare_for_shift()

Subsystems: networking [general], the rest

STALE2019d LANDED

Landed in mainline as 097b9146c0e2 on 2021-02-03.

8 messages, 4 authors, 2021-02-03 · open the first message on its own page

[PATCH net-next] net: fix up truesize of cloned skb in skb_prepare_for_shift()

From: Marco Elver <elver@google.com>
Date: 2021-02-01 16:05:55 

Avoid the assumption that ksize(kmalloc(S)) == ksize(kmalloc(S)): when
cloning an skb, save and restore truesize after pskb_expand_head(). This
can occur if the allocator decides to service an allocation of the same
size differently (e.g. use a different size class, or pass the
allocation on to KFENCE).

Because truesize is used for bookkeeping (such as sk_wmem_queued), a
modified truesize of a cloned skb may result in corrupt bookkeeping and
relevant warnings (such as in sk_stream_kill_queues()).

Link: https://lkml.kernel.org/r/X9JR/J6dMMOy1obu@elver.google.com
Reported-by: syzbot+7b99aafdcc2eedea6178@syzkaller.appspotmail.com
Suggested-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Marco Elver <elver@google.com>
---
 net/core/skbuff.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 2af12f7e170c..3787093239f5 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3289,7 +3289,19 @@ EXPORT_SYMBOL(skb_split);
  */
 static int skb_prepare_for_shift(struct sk_buff *skb)
 {
-	return skb_cloned(skb) && pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
+	int ret = 0;
+
+	if (skb_cloned(skb)) {
+		/* Save and restore truesize: pskb_expand_head() may reallocate
+		 * memory where ksize(kmalloc(S)) != ksize(kmalloc(S)), but we
+		 * cannot change truesize at this point.
+		 */
+		unsigned int save_truesize = skb->truesize;
+
+		ret = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
+		skb->truesize = save_truesize;
+	}
+	return ret;
 }
 
 /**

base-commit: 14e8e0f6008865d823a8184a276702a6c3cbef3d
-- 
2.30.0.365.g02bc693789-goog

Re: [PATCH net-next] net: fix up truesize of cloned skb in skb_prepare_for_shift()

From: Christoph Paasch <hidden>
Date: 2021-02-01 16:51:42 

On Mon, Feb 1, 2021 at 8:09 AM Marco Elver [off-list ref] wrote:
quoted hunk
Avoid the assumption that ksize(kmalloc(S)) == ksize(kmalloc(S)): when
cloning an skb, save and restore truesize after pskb_expand_head(). This
can occur if the allocator decides to service an allocation of the same
size differently (e.g. use a different size class, or pass the
allocation on to KFENCE).

Because truesize is used for bookkeeping (such as sk_wmem_queued), a
modified truesize of a cloned skb may result in corrupt bookkeeping and
relevant warnings (such as in sk_stream_kill_queues()).

Link: https://lkml.kernel.org/r/X9JR/J6dMMOy1obu@elver.google.com
Reported-by: syzbot+7b99aafdcc2eedea6178@syzkaller.appspotmail.com
Suggested-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Marco Elver <elver@google.com>
---
 net/core/skbuff.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 2af12f7e170c..3787093239f5 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3289,7 +3289,19 @@ EXPORT_SYMBOL(skb_split);
  */
 static int skb_prepare_for_shift(struct sk_buff *skb)
 {
-       return skb_cloned(skb) && pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
+       int ret = 0;
+
+       if (skb_cloned(skb)) {
+               /* Save and restore truesize: pskb_expand_head() may reallocate
+                * memory where ksize(kmalloc(S)) != ksize(kmalloc(S)), but we
+                * cannot change truesize at this point.
+                */
+               unsigned int save_truesize = skb->truesize;
+
+               ret = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
+               skb->truesize = save_truesize;
+       }
+       return ret;
just a few days ago we found out that this also fixes a syzkaller
issue on MPTCP (https://github.com/multipath-tcp/mptcp_net-next/issues/136).
I confirmed that this patch fixes the issue for us as well:

Tested-by: Christoph Paasch <redacted>





 }

 /**

base-commit: 14e8e0f6008865d823a8184a276702a6c3cbef3d
--
2.30.0.365.g02bc693789-goog

Re: [PATCH net-next] net: fix up truesize of cloned skb in skb_prepare_for_shift()

From: Marco Elver <elver@google.com>
Date: 2021-02-01 17:35:41 

On Mon, 1 Feb 2021 at 17:50, Christoph Paasch
[off-list ref] wrote:
On Mon, Feb 1, 2021 at 8:09 AM Marco Elver [off-list ref] wrote:
quoted
Avoid the assumption that ksize(kmalloc(S)) == ksize(kmalloc(S)): when
cloning an skb, save and restore truesize after pskb_expand_head(). This
can occur if the allocator decides to service an allocation of the same
size differently (e.g. use a different size class, or pass the
allocation on to KFENCE).

Because truesize is used for bookkeeping (such as sk_wmem_queued), a
modified truesize of a cloned skb may result in corrupt bookkeeping and
relevant warnings (such as in sk_stream_kill_queues()).

Link: https://lkml.kernel.org/r/X9JR/J6dMMOy1obu@elver.google.com
Reported-by: syzbot+7b99aafdcc2eedea6178@syzkaller.appspotmail.com
Suggested-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Marco Elver <elver@google.com>
---
 net/core/skbuff.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 2af12f7e170c..3787093239f5 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3289,7 +3289,19 @@ EXPORT_SYMBOL(skb_split);
  */
 static int skb_prepare_for_shift(struct sk_buff *skb)
 {
-       return skb_cloned(skb) && pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
+       int ret = 0;
+
+       if (skb_cloned(skb)) {
+               /* Save and restore truesize: pskb_expand_head() may reallocate
+                * memory where ksize(kmalloc(S)) != ksize(kmalloc(S)), but we
+                * cannot change truesize at this point.
+                */
+               unsigned int save_truesize = skb->truesize;
+
+               ret = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
+               skb->truesize = save_truesize;
+       }
+       return ret;
just a few days ago we found out that this also fixes a syzkaller
issue on MPTCP (https://github.com/multipath-tcp/mptcp_net-next/issues/136).
I confirmed that this patch fixes the issue for us as well:

Tested-by: Christoph Paasch <redacted>
That's interesting, because according to your config you did not have
KFENCE enabled. Although it's hard to say what exactly caused the
truesize mismatch in your case, because it clearly can't be KFENCE
that caused ksize(kmalloc(S))!=ksize(kmalloc(S)) for you.

Thanks,
-- Marco

Re: [PATCH net-next] net: fix up truesize of cloned skb in skb_prepare_for_shift()

From: Eric Dumazet <edumazet@google.com>
Date: 2021-02-01 17:59:24 

On Mon, Feb 1, 2021 at 6:34 PM Marco Elver [off-list ref] wrote:
On Mon, 1 Feb 2021 at 17:50, Christoph Paasch
quoted
just a few days ago we found out that this also fixes a syzkaller
issue on MPTCP (https://github.com/multipath-tcp/mptcp_net-next/issues/136).
I confirmed that this patch fixes the issue for us as well:

Tested-by: Christoph Paasch <redacted>
That's interesting, because according to your config you did not have
KFENCE enabled. Although it's hard to say what exactly caused the
truesize mismatch in your case, because it clearly can't be KFENCE
that caused ksize(kmalloc(S))!=ksize(kmalloc(S)) for you.
Indeed, this seems strange. This might be a different issue.

Maybe S != S ;)





  

    
Re: [PATCH net-next] net: fix up truesize of cloned skb in skb_prepare_for_shift()

    

      From: Christoph Paasch <hidden>

      Date: 2021-02-02 17:01:34 
    

  

  
On Mon, Feb 1, 2021 at 9:58 AM Eric Dumazet [off-list ref] wrote:
On Mon, Feb 1, 2021 at 6:34 PM Marco Elver [off-list ref] wrote:
quoted
On Mon, 1 Feb 2021 at 17:50, Christoph Paasch
quoted
quoted
just a few days ago we found out that this also fixes a syzkaller
issue on MPTCP (https://github.com/multipath-tcp/mptcp_net-next/issues/136).
I confirmed that this patch fixes the issue for us as well:

Tested-by: Christoph Paasch <redacted>
That's interesting, because according to your config you did not have
KFENCE enabled. Although it's hard to say what exactly caused the
truesize mismatch in your case, because it clearly can't be KFENCE
that caused ksize(kmalloc(S))!=ksize(kmalloc(S)) for you.
Indeed, this seems strange. This might be a different issue.

Maybe S != S ;)
Seems like letting syzkaller run for a few more days made it
eventually find the WARN again. As if Marco's change makes it harder
for us to trigger the issue.

Anyways, you can remove my "Tested-by" ;-)


Christoph





  

    
Re: [PATCH net-next] net: fix up truesize of cloned skb in skb_prepare_for_shift()

    

      From: Eric Dumazet <edumazet@google.com>

      Date: 2021-02-02 18:04:15 
    

  

  
On Mon, Feb 1, 2021 at 5:04 PM Marco Elver [off-list ref] wrote:
Avoid the assumption that ksize(kmalloc(S)) == ksize(kmalloc(S)): when
cloning an skb, save and restore truesize after pskb_expand_head(). This
can occur if the allocator decides to service an allocation of the same
size differently (e.g. use a different size class, or pass the
allocation on to KFENCE).

Because truesize is used for bookkeeping (such as sk_wmem_queued), a
modified truesize of a cloned skb may result in corrupt bookkeeping and
relevant warnings (such as in sk_stream_kill_queues()).

Link: https://lkml.kernel.org/r/X9JR/J6dMMOy1obu@elver.google.com
Reported-by: syzbot+7b99aafdcc2eedea6178@syzkaller.appspotmail.com
Suggested-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Marco Elver <elver@google.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>





  

    
Re: [PATCH net-next] net: fix up truesize of cloned skb in skb_prepare_for_shift()

    

      From: Marco Elver <elver@google.com>

      Date: 2021-02-02 18:38:13 
    

  

  
On Tue, 2 Feb 2021 at 18:59, Eric Dumazet [off-list ref] wrote:
On Mon, Feb 1, 2021 at 5:04 PM Marco Elver [off-list ref] wrote:
quoted
Avoid the assumption that ksize(kmalloc(S)) == ksize(kmalloc(S)): when
cloning an skb, save and restore truesize after pskb_expand_head(). This
can occur if the allocator decides to service an allocation of the same
size differently (e.g. use a different size class, or pass the
allocation on to KFENCE).

Because truesize is used for bookkeeping (such as sk_wmem_queued), a
modified truesize of a cloned skb may result in corrupt bookkeeping and
relevant warnings (such as in sk_stream_kill_queues()).

Link: https://lkml.kernel.org/r/X9JR/J6dMMOy1obu@elver.google.com
Reported-by: syzbot+7b99aafdcc2eedea6178@syzkaller.appspotmail.com
Suggested-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Marco Elver <elver@google.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Thank you!





  

    
Re: [PATCH net-next] net: fix up truesize of cloned skb in skb_prepare_for_shift()

    

      From: patchwork-bot+netdevbpf@kernel.org

      Date: 2021-02-03 02:01:13 
    

  

  
Hello:

This patch was applied to netdev/net-next.git (refs/heads/master):

On Mon,  1 Feb 2021 17:04:20 +0100 you wrote:
Avoid the assumption that ksize(kmalloc(S)) == ksize(kmalloc(S)): when
cloning an skb, save and restore truesize after pskb_expand_head(). This
can occur if the allocator decides to service an allocation of the same
size differently (e.g. use a different size class, or pass the
allocation on to KFENCE).

Because truesize is used for bookkeeping (such as sk_wmem_queued), a
modified truesize of a cloned skb may result in corrupt bookkeeping and
relevant warnings (such as in sk_stream_kill_queues()).

[...]
Here is the summary with links:
  - [net-next] net: fix up truesize of cloned skb in skb_prepare_for_shift()
    https://git.kernel.org/netdev/net-next/c/097b9146c0e2

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html






    
  

  
    

      

        Keyboard shortcuts
      

      

        
          
hback out one level

          
jnext message in thread

          
kprevious message in thread

          
ldrill in

          
Escclose help / fold thread tree

          
?toggle this help