| Message ID | 20210201160420.2826895-1-elver@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 097b9146c0e26aabaa6ff3e5ea536a53f5254a79 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: fix up truesize of cloned skb in skb_prepare_for_shift() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 11 of 11 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 1 this patch: 1 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 20 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1 this patch: 1 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Mon, Feb 1, 2021 at 8:09 AM Marco Elver <elver@google.com> 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> > --- > 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 <christoph.paasch@gmail.com> > } > > /** > > base-commit: 14e8e0f6008865d823a8184a276702a6c3cbef3d > -- > 2.30.0.365.g02bc693789-goog >
On Mon, 1 Feb 2021 at 17:50, Christoph Paasch <christoph.paasch@gmail.com> wrote: > On Mon, Feb 1, 2021 at 8:09 AM Marco Elver <elver@google.com> 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> > > --- > > 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 <christoph.paasch@gmail.com> 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
On Mon, Feb 1, 2021 at 6:34 PM Marco Elver <elver@google.com> wrote: > > On Mon, 1 Feb 2021 at 17:50, Christoph Paasch > > 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 <christoph.paasch@gmail.com> > > 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 ;)
On Mon, Feb 1, 2021 at 9:58 AM Eric Dumazet <edumazet@google.com> wrote: > > On Mon, Feb 1, 2021 at 6:34 PM Marco Elver <elver@google.com> wrote: > > > > On Mon, 1 Feb 2021 at 17:50, Christoph Paasch > > > > 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 <christoph.paasch@gmail.com> > > > > 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
On Mon, Feb 1, 2021 at 5:04 PM Marco Elver <elver@google.com> 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>
On Tue, 2 Feb 2021 at 18:59, Eric Dumazet <edumazet@google.com> wrote: > > On Mon, Feb 1, 2021 at 5:04 PM Marco Elver <elver@google.com> 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> Thank you!
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
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; } /**
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(-) base-commit: 14e8e0f6008865d823a8184a276702a6c3cbef3d