net: fix up truesize of cloned skb in skb_prepare_for_shift()
Subsystems (2)
PATCH 1/1 net: fix up truesize of cloned skb in skb_prepare_for_shift()
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()).
--- net/core/skbuff.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
net/core/skbuff.c +13 -1
@@ -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
Signature · 1 line
2.30.0.365.g02bc693789-goog
Quoted 31 lines · Marco Elver
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 insk_stream_kill_queues()).Reported-by [email protected]Suggested-by Eric Dumazet <[email protected]>Signed-off-by Marco Elver <[email protected]>--- net/core/skbuff.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)net/core/skbuff.c +3
-1@@ -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)) {
net/core/skbuff.c EXPORT_SYMBOL(skb_split); · line 3304
Quoted 10 lines
net/core/skbuff.c +10
-0+/* 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:
Quoted 8 lines
net/core/skbuff.c +0
-0}/** base-commit:
14e8e0f6008865d823a8184a276702a6c3cbef3d--
2.30.0.365.g02bc693789-goog
Quoted 47 lines · Christoph Paasch
Quoted 31 lines · Marco Elver
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 insk_stream_kill_queues()).Reported-by [email protected]Suggested-by Eric Dumazet <[email protected]>Signed-off-by Marco Elver <[email protected]>--- net/core/skbuff.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)net/core/skbuff.c +3
-1@@ -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)) {net/core/skbuff.c EXPORT_SYMBOL(skb_split); · line 3304
Quoted 10 lines
net/core/skbuff.c +10
-0+/* 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:
Quoted 1 line
Tested-by Christoph Paasch <[email protected]>
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
Quoted 7 lines · Marco Elver
On Mon, 1 Feb 2021 at 17:50, Christoph Paasch
Quoted 4 lines
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:
Quoted 6 lines
Quoted 1 line
Tested-by Christoph Paasch <[email protected]>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 ;)
Quoted 11 lines · Eric Dumazet
Quoted 9 lines · Marco Elver
On Mon, 1 Feb 2021 at 17:50, Christoph Paasch
Quoted 5 lines
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 <[email protected]>
Quoted 8 lines
Quoted 4 lines
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
Quoted 7 lines · Marco Elver
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).
Quoted 8 lines
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 insk_stream_kill_queues()).Reported-by [email protected]Suggested-by Eric Dumazet <[email protected]>Signed-off-by Marco Elver <[email protected]>
Quoted 13 lines · Eric Dumazet
Quoted 11 lines · Marco Elver
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 insk_stream_kill_queues()).
Quoted 6 lines
Quoted 4 lines
Reported-by [email protected]Suggested-by Eric Dumazet <[email protected]>Signed-off-by Marco Elver <[email protected]>Signed-off-by Eric Dumazet <[email protected]>
Thank you!
This patch was applied to netdev/net-next.git (refs/heads/master):
Hello:
This patch was applied to netdev/net-next.git (refs/heads/master):
Quoted 11 lines · 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 insk_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