crash in __kfree_skb on v3.18-rc5 with CONFIG_DEBUG_PAGEALLOC

crash in __kfree_skb on v3.18-rc5 with CONFIG_DEBUG_PAGEALLOC

[Chris Mason]

Hi everyone,

I've hit this a few times today while hammering on my btrfs queue for
the next merge window.  It's plain v3.18-rc5 plus a few btrfs patches,
so it isn't impossible a btrfs double free is causing trouble.

But, that should also show up in places outside the networking stack and I've
gotten this exact stack trace twice now:

[ 2255.152925] BUG: unable to handle kernel paging request at ffff880fa1f91f96
[ 2255.185251]  [<ffffffff81595f68>] __kfree_skb+0x58/0xc0
[ 2255.196223] PGD 2be4067 PUD 10783cb067 PMD 10782bb067 PTE 8000000fa1f91060
[ 2255.210163] Oops: 0002 [#1] SMP DEBUG_PAGEALLOC
[ 2255.219394] Modules linked in: btrfs raid6_pq zlib_deflate lzo_compress xor xfs exportfs libcrc32c nfsv4 fuse k10temp coretemp hwmon tcp_diag inet_diag loop ip6table_filter ip6_tables xt_NFLOG nfnetlink_log nfnetlink xt_comment xt_statistic iptable_filter ip_tables x_tables nfsv3 nfs lockd grace mptctl netconsole autofs4 rpcsec_gss_krb5 auth_rpcgss oid_registry sunrpc ipv6 ext3 jbd dm_mod iTCO_wdt iTCO_vendor_support rtc_cmos ipmi_si ipmi_msghandler pcspkr i2c_i801 lpc_ich mfd_core shpchp ehci_pci ehci_hcd mlx4_en ptp pps_core mlx4_core ses enclosure sg button megaraid_sas
[ 2255.323468] CPU: 14 PID: 8517 Comm: scribe-event Not tainted 3.18.0-rc5-mason+ #62
[ 2255.338754] Hardware name: ZTSYSTEMS Echo Ridge T4  /A9DRPF-10D, BIOS 1.07 05/10/2012
[ 2255.354557] task: ffff881018b61d10 ti: ffff880ff6ae4000 task.ti: ffff880ff6ae4000
[ 2255.369680] RIP: 0010:[<ffffffff81595f68>]  [<ffffffff81595f68>] __kfree_skb+0x58/0xc0
[ 2255.385709] RSP: 0018:ffff880ff6ae7b98  EFLAGS: 00010202
[ 2255.396398] RAX: 0000000000000002 RBX: ffff880fa1f91f18 RCX: ffffffff81cd5d80
[ 2255.410728] RDX: 00000000ffffffff RSI: ffff880fa1f91e40 RDI: ffff880fa1f91f18
[ 2255.425062] RBP: ffff880ff6ae7ba8 R08: 000000000000001b R09: 0000000000000000
[ 2255.439379] R10: ffff8810385ef640 R11: ffff8810385ef758 R12: 0000000000000000
[ 2255.453702] R13: ffff880fa1f91f40 R14: 0000000000000000 R15: ffff8810385efd4c
[ 2255.468024] FS:  00007ff18ebff700(0000) GS:ffff881077cc0000(0000) knlGS:0000000000000000
[ 2255.484321] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 2255.495864] CR2: ffff880fa1f91f96 CR3: 0000000850174000 CR4: 00000000000407e0
[ 2255.510188] Stack:
[ 2255.514279]  0000000000000000 ffff880fa1f91f18 ffff880ff6ae7ca8 ffffffff815f27aa
[ 2255.529306]  ffff881018b61d10 0000000000000001 000000000000001b ffff8810385ef640
[ 2255.544337]  ffff8810385ef758 ffff8810385ef7a8 ffff881018b61d10 0000000000000000
[ 2255.559369] Call Trace:
[ 2255.564332]  [<ffffffff815f27aa>] tcp_recvmsg+0xa2a/0xd10
[ 2255.575198]  [<ffffffff8161dcb1>] inet_recvmsg+0xe1/0x110
[ 2255.586056]  [<ffffffff8158c573>] sock_recvmsg+0xa3/0xd0
[ 2255.596740]  [<ffffffff811c9e65>] ? __fget_light+0x25/0x60
[ 2255.607768]  [<ffffffff8158c664>] SYSC_recvfrom+0xc4/0x130
[ 2255.618801]  [<ffffffff810e987c>] ? __audit_syscall_entry+0xac/0x110
[ 2255.631566]  [<ffffffff810b6635>] ? current_kernel_time+0x95/0xb0
[ 2255.643826]  [<ffffffff8109537d>] ? trace_hardirqs_on_caller+0xfd/0x1c0
[ 2255.657122]  [<ffffffff8158c6de>] SyS_recvfrom+0xe/0x10
[ 2255.667632]  [<ffffffff81670b92>] system_call_fastpath+0x12/0x17
[ 2255.679699] Code: 0f 48 89 de 48 8b 3d 58 08 76 00 e8 33 a6 bf ff 48 83 c4 08 5b c9 c3 0f 1f 40 00 48 8d b3 28 ff ff ff f0 ff 8e b0 01 00 00 74 48 <80> 4b 7e 0c 48 83 c4 08 5b c9 c3 0f 1f 44 00 00 f0 ff 8b b0 01 
[ 2255.719771] RIP  [<ffffffff81595f68>] __kfree_skb+0x58/0xc0
[ 2255.731019]  RSP <ffff880ff6ae7b98>
[ 2255.738081] CR2: ffff880fa1f91f96
[ 2255.745371] ---[ end trace 982fb6dd92d9b65b ]---

Which translates to:

0xffffffff81595f68 is in __kfree_skb (net/core/skbuff.c:567).
562				kmem_cache_free(skbuff_fclone_cache, fclones);
563			} else {
564				/* The clone portion is available for
565				 * fast-cloning again.
566				 */
567				skb->fclone = SKB_FCLONE_FREE;
568			}
569			break;
570		}
571	}

Just looking for related code in the changelog, this one might be
related:

commit c8753d55afb436fd6a25c8bbe8d783f6dcf1c9f8
Author: Vijay Subramanian <subramanian.vijay@gmail.com>
Date:   Thu Oct 2 10:00:43 2014 -0700

    net: Cleanup skb cloning by adding SKB_FCLONE_FREE

I'm not hitting this consistently enough for a revert or a bisect to
prove anything.

-chris

Re: crash in __kfree_skb on v3.18-rc5 with CONFIG_DEBUG_PAGEALLOC

[Eric Dumazet]

On Fri, 2014-11-21 at 11:16 -0500, Chris Mason wrote:
> Hi everyone,
> 
> I've hit this a few times today while hammering on my btrfs queue for
> the next merge window.  It's plain v3.18-rc5 plus a few btrfs patches,
> so it isn't impossible a btrfs double free is causing trouble.
> 
> But, that should also show up in places outside the networking stack and I've
> gotten this exact stack trace twice now:
> 
> [ 2255.152925] BUG: unable to handle kernel paging request at ffff880fa1f91f96
> [ 2255.185251]  [<ffffffff81595f68>] __kfree_skb+0x58/0xc0
> [ 2255.196223] PGD 2be4067 PUD 10783cb067 PMD 10782bb067 PTE 8000000fa1f91060
> [ 2255.210163] Oops: 0002 [#1] SMP DEBUG_PAGEALLOC
> [ 2255.219394] Modules linked in: btrfs raid6_pq zlib_deflate lzo_compress xor xfs exportfs libcrc32c nfsv4 fuse k10temp coretemp hwmon tcp_diag inet_diag loop ip6table_filter ip6_tables xt_NFLOG nfnetlink_log nfnetlink xt_comment xt_statistic iptable_filter ip_tables x_tables nfsv3 nfs lockd grace mptctl netconsole autofs4 rpcsec_gss_krb5 auth_rpcgss oid_registry sunrpc ipv6 ext3 jbd dm_mod iTCO_wdt iTCO_vendor_support rtc_cmos ipmi_si ipmi_msghandler pcspkr i2c_i801 lpc_ich mfd_core shpchp ehci_pci ehci_hcd mlx4_en ptp pps_core mlx4_core ses enclosure sg button megaraid_sas
> [ 2255.323468] CPU: 14 PID: 8517 Comm: scribe-event Not tainted 3.18.0-rc5-mason+ #62
> [ 2255.338754] Hardware name: ZTSYSTEMS Echo Ridge T4  /A9DRPF-10D, BIOS 1.07 05/10/2012
> [ 2255.354557] task: ffff881018b61d10 ti: ffff880ff6ae4000 task.ti: ffff880ff6ae4000
> [ 2255.369680] RIP: 0010:[<ffffffff81595f68>]  [<ffffffff81595f68>] __kfree_skb+0x58/0xc0
> [ 2255.385709] RSP: 0018:ffff880ff6ae7b98  EFLAGS: 00010202
> [ 2255.396398] RAX: 0000000000000002 RBX: ffff880fa1f91f18 RCX: ffffffff81cd5d80
> [ 2255.410728] RDX: 00000000ffffffff RSI: ffff880fa1f91e40 RDI: ffff880fa1f91f18
> [ 2255.425062] RBP: ffff880ff6ae7ba8 R08: 000000000000001b R09: 0000000000000000
> [ 2255.439379] R10: ffff8810385ef640 R11: ffff8810385ef758 R12: 0000000000000000
> [ 2255.453702] R13: ffff880fa1f91f40 R14: 0000000000000000 R15: ffff8810385efd4c
> [ 2255.468024] FS:  00007ff18ebff700(0000) GS:ffff881077cc0000(0000) knlGS:0000000000000000
> [ 2255.484321] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 2255.495864] CR2: ffff880fa1f91f96 CR3: 0000000850174000 CR4: 00000000000407e0
> [ 2255.510188] Stack:
> [ 2255.514279]  0000000000000000 ffff880fa1f91f18 ffff880ff6ae7ca8 ffffffff815f27aa
> [ 2255.529306]  ffff881018b61d10 0000000000000001 000000000000001b ffff8810385ef640
> [ 2255.544337]  ffff8810385ef758 ffff8810385ef7a8 ffff881018b61d10 0000000000000000
> [ 2255.559369] Call Trace:
> [ 2255.564332]  [<ffffffff815f27aa>] tcp_recvmsg+0xa2a/0xd10
> [ 2255.575198]  [<ffffffff8161dcb1>] inet_recvmsg+0xe1/0x110
> [ 2255.586056]  [<ffffffff8158c573>] sock_recvmsg+0xa3/0xd0
> [ 2255.596740]  [<ffffffff811c9e65>] ? __fget_light+0x25/0x60
> [ 2255.607768]  [<ffffffff8158c664>] SYSC_recvfrom+0xc4/0x130
> [ 2255.618801]  [<ffffffff810e987c>] ? __audit_syscall_entry+0xac/0x110
> [ 2255.631566]  [<ffffffff810b6635>] ? current_kernel_time+0x95/0xb0
> [ 2255.643826]  [<ffffffff8109537d>] ? trace_hardirqs_on_caller+0xfd/0x1c0
> [ 2255.657122]  [<ffffffff8158c6de>] SyS_recvfrom+0xe/0x10
> [ 2255.667632]  [<ffffffff81670b92>] system_call_fastpath+0x12/0x17
> [ 2255.679699] Code: 0f 48 89 de 48 8b 3d 58 08 76 00 e8 33 a6 bf ff 48 83 c4 08 5b c9 c3 0f 1f 40 00 48 8d b3 28 ff ff ff f0 ff 8e b0 01 00 00 74 48 <80> 4b 7e 0c 48 83 c4 08 5b c9 c3 0f 1f 44 00 00 f0 ff 8b b0 01 
> [ 2255.719771] RIP  [<ffffffff81595f68>] __kfree_skb+0x58/0xc0
> [ 2255.731019]  RSP <ffff880ff6ae7b98>
> [ 2255.738081] CR2: ffff880fa1f91f96
> [ 2255.745371] ---[ end trace 982fb6dd92d9b65b ]---
> 
> Which translates to:
> 
> 0xffffffff81595f68 is in __kfree_skb (net/core/skbuff.c:567).
> 562				kmem_cache_free(skbuff_fclone_cache, fclones);
> 563			} else {
> 564				/* The clone portion is available for
> 565				 * fast-cloning again.
> 566				 */
> 567				skb->fclone = SKB_FCLONE_FREE;
> 568			}
> 569			break;
> 570		}
> 571	}
> 
> Just looking for related code in the changelog, this one might be
> related:
> 
> commit c8753d55afb436fd6a25c8bbe8d783f6dcf1c9f8
> Author: Vijay Subramanian <subramanian.vijay@gmail.com>
> Date:   Thu Oct 2 10:00:43 2014 -0700
> 
>     net: Cleanup skb cloning by adding SKB_FCLONE_FREE
> 
> I'm not hitting this consistently enough for a revert or a bisect to
> prove anything.

Hi Chris

Can you double check, or send whole __kfree_skb() disassembly ?

I do not understand how skb->fclone could possibly trap _at_ this point.

Re: crash in __kfree_skb on v3.18-rc5 with CONFIG_DEBUG_PAGEALLOC

[Chris Mason]

On Fri, Nov 21, 2014 at 11:16 AM, Chris Mason <clm@fb.com> wrote:
> Hi everyone,
> 
> I've hit this a few times today while hammering on my btrfs queue for
> the next merge window.  It's plain v3.18-rc5 plus a few btrfs patches,
> so it isn't impossible a btrfs double free is causing trouble.

Looking through my console logs, I also hit it on v3.18-rc3 and on a 
test in the middle of the 3.18 merge window (don't have the exact sha, 
sorry).

-chris

Re: crash in __kfree_skb on v3.18-rc5 with CONFIG_DEBUG_PAGEALLOC

[Eric Dumazet]

On Fri, 2014-11-21 at 08:31 -0800, Eric Dumazet wrote:

> Can you double check, or send whole __kfree_skb() disassembly ?
> 
> I do not understand how skb->fclone could possibly trap _at_ this point.

Oh well, I think commit ce1a4ea3f1258 ("net: avoid one atomic operation
in skb_clone()") is the problem, I'll send a revert.

Re: crash in __kfree_skb on v3.18-rc5 with CONFIG_DEBUG_PAGEALLOC

[Chris Mason]

On Fri, Nov 21, 2014 at 11:31 AM, Eric Dumazet <eric.dumazet@gmail.com> 
wrote:
> On Fri, 2014-11-21 at 11:16 -0500, Chris Mason wrote:
>>  Hi everyone,
>> 
>>  I've hit this a few times today while hammering on my btrfs queue 
>> for
>>  the next merge window.  It's plain v3.18-rc5 plus a few btrfs 
>> patches,
>>  so it isn't impossible a btrfs double free is causing trouble.
>> 
>>  But, that should also show up in places outside the networking 
>> stack and I've
>>  gotten this exact stack trace twice now:
>> 
>>  [ 2255.152925] BUG: unable to handle kernel paging request at 
>> ffff880fa1f91f96
>>  [ 2255.185251]  [<ffffffff81595f68>] __kfree_skb+0x58/0xc0
>>  [ 2255.196223] PGD 2be4067 PUD 10783cb067 PMD 10782bb067 PTE 
>> 8000000fa1f91060
>>  [ 2255.210163] Oops: 0002 [#1] SMP DEBUG_PAGEALLOC
>>  [ 2255.219394] Modules linked in: btrfs raid6_pq zlib_deflate 
>> lzo_compress xor xfs exportfs libcrc32c nfsv4 fuse k10temp coretemp 
>> hwmon tcp_diag inet_diag loop ip6table_filter ip6_tables xt_NFLOG 
>> nfnetlink_log nfnetlink xt_comment xt_statistic iptable_filter 
>> ip_tables x_tables nfsv3 nfs lockd grace mptctl netconsole autofs4 
>> rpcsec_gss_krb5 auth_rpcgss oid_registry sunrpc ipv6 ext3 jbd dm_mod 
>> iTCO_wdt iTCO_vendor_support rtc_cmos ipmi_si ipmi_msghandler pcspkr 
>> i2c_i801 lpc_ich mfd_core shpchp ehci_pci ehci_hcd mlx4_en ptp 
>> pps_core mlx4_core ses enclosure sg button megaraid_sas
>>  [ 2255.323468] CPU: 14 PID: 8517 Comm: scribe-event Not tainted 
>> 3.18.0-rc5-mason+ #62
>>  [ 2255.338754] Hardware name: ZTSYSTEMS Echo Ridge T4  /A9DRPF-10D, 
>> BIOS 1.07 05/10/2012
>>  [ 2255.354557] task: ffff881018b61d10 ti: ffff880ff6ae4000 task.ti: 
>> ffff880ff6ae4000
>>  [ 2255.369680] RIP: 0010:[<ffffffff81595f68>]  [<ffffffff81595f68>] 
>> __kfree_skb+0x58/0xc0
>>  [ 2255.385709] RSP: 0018:ffff880ff6ae7b98  EFLAGS: 00010202
>>  [ 2255.396398] RAX: 0000000000000002 RBX: ffff880fa1f91f18 RCX: 
>> ffffffff81cd5d80
>>  [ 2255.410728] RDX: 00000000ffffffff RSI: ffff880fa1f91e40 RDI: 
>> ffff880fa1f91f18
>>  [ 2255.425062] RBP: ffff880ff6ae7ba8 R08: 000000000000001b R09: 
>> 0000000000000000
>>  [ 2255.439379] R10: ffff8810385ef640 R11: ffff8810385ef758 R12: 
>> 0000000000000000
>>  [ 2255.453702] R13: ffff880fa1f91f40 R14: 0000000000000000 R15: 
>> ffff8810385efd4c
>>  [ 2255.468024] FS:  00007ff18ebff700(0000) 
>> GS:ffff881077cc0000(0000) knlGS:0000000000000000
>>  [ 2255.484321] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>  [ 2255.495864] CR2: ffff880fa1f91f96 CR3: 0000000850174000 CR4: 
>> 00000000000407e0
>>  [ 2255.510188] Stack:
>>  [ 2255.514279]  0000000000000000 ffff880fa1f91f18 ffff880ff6ae7ca8 
>> ffffffff815f27aa
>>  [ 2255.529306]  ffff881018b61d10 0000000000000001 000000000000001b 
>> ffff8810385ef640
>>  [ 2255.544337]  ffff8810385ef758 ffff8810385ef7a8 ffff881018b61d10 
>> 0000000000000000
>>  [ 2255.559369] Call Trace:
>>  [ 2255.564332]  [<ffffffff815f27aa>] tcp_recvmsg+0xa2a/0xd10
>>  [ 2255.575198]  [<ffffffff8161dcb1>] inet_recvmsg+0xe1/0x110
>>  [ 2255.586056]  [<ffffffff8158c573>] sock_recvmsg+0xa3/0xd0
>>  [ 2255.596740]  [<ffffffff811c9e65>] ? __fget_light+0x25/0x60
>>  [ 2255.607768]  [<ffffffff8158c664>] SYSC_recvfrom+0xc4/0x130
>>  [ 2255.618801]  [<ffffffff810e987c>] ? 
>> __audit_syscall_entry+0xac/0x110
>>  [ 2255.631566]  [<ffffffff810b6635>] ? current_kernel_time+0x95/0xb0
>>  [ 2255.643826]  [<ffffffff8109537d>] ? 
>> trace_hardirqs_on_caller+0xfd/0x1c0
>>  [ 2255.657122]  [<ffffffff8158c6de>] SyS_recvfrom+0xe/0x10
>>  [ 2255.667632]  [<ffffffff81670b92>] system_call_fastpath+0x12/0x17
>>  [ 2255.679699] Code: 0f 48 89 de 48 8b 3d 58 08 76 00 e8 33 a6 bf 
>> ff 48 83 c4 08 5b c9 c3 0f 1f 40 00 48 8d b3 28 ff ff ff f0 ff 8e b0 
>> 01 00 00 74 48 <80> 4b 7e 0c 48 83 c4 08 5b c9 c3 0f 1f 44 00 00 f0 
>> ff 8b b0 01
>>  [ 2255.719771] RIP  [<ffffffff81595f68>] __kfree_skb+0x58/0xc0
>>  [ 2255.731019]  RSP <ffff880ff6ae7b98>
>>  [ 2255.738081] CR2: ffff880fa1f91f96
>>  [ 2255.745371] ---[ end trace 982fb6dd92d9b65b ]---
>> 
>>  Which translates to:
>> 
>>  0xffffffff81595f68 is in __kfree_skb (net/core/skbuff.c:567).
>>  562				kmem_cache_free(skbuff_fclone_cache, fclones);
>>  563			} else {
>>  564				/* The clone portion is available for
>>  565				 * fast-cloning again.
>>  566				 */
>>  567				skb->fclone = SKB_FCLONE_FREE;
>>  568			}
>>  569			break;
>>  570		}
>>  571	}
>> 
>>  Just looking for related code in the changelog, this one might be
>>  related:
>> 
>>  commit c8753d55afb436fd6a25c8bbe8d783f6dcf1c9f8
>>  Author: Vijay Subramanian <subramanian.vijay@gmail.com>
>>  Date:   Thu Oct 2 10:00:43 2014 -0700
>> 
>>      net: Cleanup skb cloning by adding SKB_FCLONE_FREE
>> 
>>  I'm not hitting this consistently enough for a revert or a bisect to
>>  prove anything.
> 
> Hi Chris
> 
> Can you double check, or send whole __kfree_skb() disassembly ?
> 
> I do not understand how skb->fclone could possibly trap _at_ this 
> point.

I'm running with CONFIG_DEBUG_PAGEALLOC, so skb is in a page that has 
been freed.  We're crashing just because we touched it.

-chris

Re: crash in __kfree_skb on v3.18-rc5 with CONFIG_DEBUG_PAGEALLOC

[Chris Mason]

On Fri, Nov 21, 2014 at 11:37 AM, Eric Dumazet <eric.dumazet@gmail.com> 
wrote:
> On Fri, 2014-11-21 at 08:31 -0800, Eric Dumazet wrote:
> 
>>  Can you double check, or send whole __kfree_skb() disassembly ?
>> 
>>  I do not understand how skb->fclone could possibly trap _at_ this 
>> point.
> 
> Oh well, I think commit ce1a4ea3f1258 ("net: avoid one atomic 
> operation
> in skb_clone()") is the problem, I'll send a revert.

Once my current run is done, I can revert this and try a loop.  But I 
don't see how this commit causes a use-after-free on the skb itself?

-chris

Re: crash in __kfree_skb on v3.18-rc5 with CONFIG_DEBUG_PAGEALLOC

[Eric Dumazet]

On Fri, 2014-11-21 at 11:47 -0500, Chris Mason wrote:

> Once my current run is done, I can revert this and try a loop.  But I 
> don't see how this commit causes a use-after-free on the skb itself?

Well, it is so obvious, you really want to make me miserable ;)

I am testing the revert and send it asap.

Re: crash in __kfree_skb on v3.18-rc5 with CONFIG_DEBUG_PAGEALLOC

[Chris Mason]

On Fri, Nov 21, 2014 at 11:31 AM, Eric Dumazet <eric.dumazet@gmail.com> 
wrote:
> On Fri, 2014-11-21 at 11:16 -0500, Chris Mason wrote:
>>  Hi everyone,
>> 
>>  I've hit this a few times today while hammering on my btrfs queue 
>> for
>>  the next merge window.  It's plain v3.18-rc5 plus a few btrfs 
>> patches,
>>  so it isn't impossible a btrfs double free is causing trouble.
>> 
>>  But, that should also show up in places outside the networking 
>> stack and I've
>>  gotten this exact stack trace twice now:
>> 
>>  [ 2255.152925] BUG: unable to handle kernel paging request at 
>> ffff880fa1f91f96
>>  [ 2255.185251]  [<ffffffff81595f68>] __kfree_skb+0x58/0xc0Hi Chris
> 
> Can you double check, or send whole __kfree_skb() disassembly ?
> 
> I do not understand how skb->fclone could possibly trap _at_ this 
> point.

So I double checked and got worried about the orb instruction until I 
realized fclone is in a bitfield:

Dump of assembler code for function __kfree_skb:
   0xffffffff81595f10 <+0>:	push   %rbp
   0xffffffff81595f11 <+1>:	mov    %rsp,%rbp
   0xffffffff81595f14 <+4>:	push   %rbx
   0xffffffff81595f15 <+5>:	sub    $0x8,%rsp
   0xffffffff81595f19 <+9>:	callq  0xffffffff81672c40 <mcount>
   0xffffffff81595f1e <+14>:	mov    %rdi,%rbx
   0xffffffff81595f21 <+17>:	callq  0xffffffff81595ee0 <skb_release_all>
   0xffffffff81595f26 <+22>:	movzbl 0x7e(%rbx),%eax
   0xffffffff81595f2a <+26>:	shr    $0x2,%al
   0xffffffff81595f2d <+29>:	and    $0x3,%eax
   0xffffffff81595f30 <+32>:	cmp    $0x1,%eax
   0xffffffff81595f33 <+35>:	je     0xffffffff81595f78 <__kfree_skb+104>
   0xffffffff81595f35 <+37>:	cmp    $0x2,%eax
   0xffffffff81595f38 <+40>:	je     0xffffffff81595f58 <__kfree_skb+72>
   0xffffffff81595f3a <+42>:	test   %eax,%eax
   0xffffffff81595f3c <+44>:	jne    0xffffffff81595f4d <__kfree_skb+61>
   0xffffffff81595f3e <+46>:	mov    %rbx,%rsi
   0xffffffff81595f41 <+49>:	mov    0x760858(%rip),%rdi        # 
0xffffffff81cf67a0 <skbuff_head_cache>
   0xffffffff81595f48 <+56>:	callq  0xffffffff81190580 <kmem_cache_free>
   0xffffffff81595f4d <+61>:	add    $0x8,%rsp
   0xffffffff81595f51 <+65>:	pop    %rbx
   0xffffffff81595f52 <+66>:	leaveq
   0xffffffff81595f53 <+67>:	retq
   0xffffffff81595f54 <+68>:	nopl   0x0(%rax)
   0xffffffff81595f58 <+72>:	lea    -0xd8(%rbx),%rsi
   0xffffffff81595f5f <+79>:	lock decl 0x1b0(%rsi)
   0xffffffff81595f66 <+86>:	je     0xffffffff81595fb0 <__kfree_skb+160>
   0xffffffff81595f68 <+88>:	orb    $0xc,0x7e(%rbx)

^^^^^^^^^^^^^^^^^^^^^
Should be skb->fclone = SKB_FCLONE_FREE;

   0xffffffff81595f6c <+92>:	add    $0x8,%rsp
   0xffffffff81595f70 <+96>:	pop    %rbx
   0xffffffff81595f71 <+97>:	leaveq
   0xffffffff81595f72 <+98>:	retq
   0xffffffff81595f73 <+99>:	nopl   0x0(%rax,%rax,1)
   0xffffffff81595f78 <+104>:	lock decl 0x1b0(%rbx)
   0xffffffff81595f7f <+111>:	je     0xffffffff81595f90 
<__kfree_skb+128>
   0xffffffff81595f81 <+113>:	add    $0x8,%rsp
   0xffffffff81595f85 <+117>:	pop    %rbx
   0xffffffff81595f86 <+118>:	leaveq
   0xffffffff81595f87 <+119>:	retq
   0xffffffff81595f88 <+120>:	nopl   0x0(%rax,%rax,1)
   0xffffffff81595f90 <+128>:	mov    %rbx,%rsi
   0xffffffff81595f93 <+131>:	mov    0x7607fe(%rip),%rdi        # 
0xffffffff81cf6798 <skbuff_fclone_cache>
   0xffffffff81595f9a <+138>:	callq  0xffffffff81190580 
<kmem_cache_free>
   0xffffffff81595f9f <+143>:	add    $0x8,%rsp
   0xffffffff81595fa3 <+147>:	pop    %rbx
   0xffffffff81595fa4 <+148>:	leaveq
   0xffffffff81595fa5 <+149>:	retq
   0xffffffff81595fa6 <+150>:	nopw   %cs:0x0(%rax,%rax,1)
   0xffffffff81595fb0 <+160>:	mov    0x7607e1(%rip),%rdi        # 
0xffffffff81cf6798 <skbuff_fclone_cache>
   0xffffffff81595fb7 <+167>:	callq  0xffffffff81190580 
<kmem_cache_free>
   0xffffffff81595fbc <+172>:	add    $0x8,%rsp
   0xffffffff81595fc0 <+176>:	pop    %rbx
   0xffffffff81595fc1 <+177>:	leaveq
   0xffffffff81595fc2 <+178>:	retq

-chris

[PATCH net] net: Revert "net: avoid one atomic operation in skb_clone()"

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Not sure what I was thinking, but doing anything after
releasing a refcount is suicidal or/and embarrassing.

By the time we set skb->fclone to SKB_FCLONE_FREE, another cpu
could have released last reference and freed whole skb.

We potentially corrupt memory or trap if CONFIG_DEBUG_PAGEALLOC is set.

Reported-by: Chris Mason <clm@fb.com>
Fixes: ce1a4ea3f1258 ("net: avoid one atomic operation in skb_clone()")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/core/skbuff.c |   23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index c16615bfb61e..be4c7deed971 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -552,20 +552,13 @@ static void kfree_skbmem(struct sk_buff *skb)
 	case SKB_FCLONE_CLONE:
 		fclones = container_of(skb, struct sk_buff_fclones, skb2);
 
-		/* Warning : We must perform the atomic_dec_and_test() before
-		 * setting skb->fclone back to SKB_FCLONE_FREE, otherwise
-		 * skb_clone() could set clone_ref to 2 before our decrement.
-		 * Anyway, if we are going to free the structure, no need to
-		 * rewrite skb->fclone.
+		/* The clone portion is available for
+		 * fast-cloning again.
 		 */
-		if (atomic_dec_and_test(&fclones->fclone_ref)) {
+		skb->fclone = SKB_FCLONE_UNAVAILABLE;
+
+		if (atomic_dec_and_test(&fclones->fclone_ref))
 			kmem_cache_free(skbuff_fclone_cache, fclones);
-		} else {
-			/* The clone portion is available for
-			 * fast-cloning again.
-			 */
-			skb->fclone = SKB_FCLONE_FREE;
-		}
 		break;
 	}
 }
@@ -887,11 +880,7 @@ struct sk_buff *skb_clone(struct sk_buff *skb, gfp_t gfp_mask)
 	if (skb->fclone == SKB_FCLONE_ORIG &&
 	    n->fclone == SKB_FCLONE_FREE) {
 		n->fclone = SKB_FCLONE_CLONE;
-		/* As our fastclone was free, clone_ref must be 1 at this point.
-		 * We could use atomic_inc() here, but it is faster
-		 * to set the final value.
-		 */
-		atomic_set(&fclones->fclone_ref, 2);
+		atomic_inc(&fclones->fclone_ref);
 	} else {
 		if (skb_pfmemalloc(skb))
 			gfp_mask |= __GFP_MEMALLOC;

Re: crash in __kfree_skb on v3.18-rc5 with CONFIG_DEBUG_PAGEALLOC

[Chris Mason]

On Fri, Nov 21, 2014 at 11:56 AM, Eric Dumazet <eric.dumazet@gmail.com> 
wrote:
> On Fri, 2014-11-21 at 11:47 -0500, Chris Mason wrote:
> 
>>  Once my current run is done, I can revert this and try a loop.  But 
>> I
>>  don't see how this commit causes a use-after-free on the skb itself?
> 
> Well, it is so obvious, you really want to make me miserable ;)
> 
> I am testing the revert and send it asap.

Grin, no I just don't know what fclones really are ;)  So dropping the 
ref by one on the clone opens up the skb to be freed and another CPU 
races in and frees skb before we can do the assignment lower down.

I've added your revert here and I'll restart the test.  It takes a few 
hours to get to the point where I've fallen over so far today, but I'll 
just leave it in a loop all day long.

Thanks!

-chris

Re: [PATCH net] net: Revert "net: avoid one atomic operation in skb_clone()"

[Sabrina Dubroca]

Hello Eric,

2014-11-21, 09:04:11 -0800, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> Not sure what I was thinking, but doing anything after
> releasing a refcount is suicidal or/and embarrassing.
> 
> By the time we set skb->fclone to SKB_FCLONE_FREE, another cpu
> could have released last reference and freed whole skb.
> 
> We potentially corrupt memory or trap if CONFIG_DEBUG_PAGEALLOC is set.
> 
> Reported-by: Chris Mason <clm@fb.com>
> Fixes: ce1a4ea3f1258 ("net: avoid one atomic operation in skb_clone()")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/core/skbuff.c |   23 ++++++-----------------
>  1 file changed, 6 insertions(+), 17 deletions(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index c16615bfb61e..be4c7deed971 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -552,20 +552,13 @@ static void kfree_skbmem(struct sk_buff *skb)
>  	case SKB_FCLONE_CLONE:
>  		fclones = container_of(skb, struct sk_buff_fclones, skb2);
>  
> -		/* Warning : We must perform the atomic_dec_and_test() before
> -		 * setting skb->fclone back to SKB_FCLONE_FREE, otherwise
> -		 * skb_clone() could set clone_ref to 2 before our decrement.
> -		 * Anyway, if we are going to free the structure, no need to
> -		 * rewrite skb->fclone.
> +		/* The clone portion is available for
> +		 * fast-cloning again.
>  		 */
> -		if (atomic_dec_and_test(&fclones->fclone_ref)) {
> +		skb->fclone = SKB_FCLONE_UNAVAILABLE;

Shouldn't that be SKB_FCLONE_FREE ?

> +
> +		if (atomic_dec_and_test(&fclones->fclone_ref))
>  			kmem_cache_free(skbuff_fclone_cache, fclones);
> -		} else {
> -			/* The clone portion is available for
> -			 * fast-cloning again.
> -			 */
> -			skb->fclone = SKB_FCLONE_FREE;

like here                             ^^^^



Thanks,

-- 
Sabrina

Re: [PATCH net] net: Revert "net: avoid one atomic operation in skb_clone()"

[Eric Dumazet]

On Fri, 2014-11-21 at 19:05 +0100, Sabrina Dubroca wrote:
> Hello Eric,
> 
> 2014-11-21, 09:04:11 -0800, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> > 
> > Not sure what I was thinking, but doing anything after
> > releasing a refcount is suicidal or/and embarrassing.
> > 
> > By the time we set skb->fclone to SKB_FCLONE_FREE, another cpu
> > could have released last reference and freed whole skb.
> > 
> > We potentially corrupt memory or trap if CONFIG_DEBUG_PAGEALLOC is set.
> > 
> > Reported-by: Chris Mason <clm@fb.com>
> > Fixes: ce1a4ea3f1258 ("net: avoid one atomic operation in skb_clone()")
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > ---
> >  net/core/skbuff.c |   23 ++++++-----------------
> >  1 file changed, 6 insertions(+), 17 deletions(-)
> > 
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index c16615bfb61e..be4c7deed971 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -552,20 +552,13 @@ static void kfree_skbmem(struct sk_buff *skb)
> >  	case SKB_FCLONE_CLONE:
> >  		fclones = container_of(skb, struct sk_buff_fclones, skb2);
> >  
> > -		/* Warning : We must perform the atomic_dec_and_test() before
> > -		 * setting skb->fclone back to SKB_FCLONE_FREE, otherwise
> > -		 * skb_clone() could set clone_ref to 2 before our decrement.
> > -		 * Anyway, if we are going to free the structure, no need to
> > -		 * rewrite skb->fclone.
> > +		/* The clone portion is available for
> > +		 * fast-cloning again.
> >  		 */
> > -		if (atomic_dec_and_test(&fclones->fclone_ref)) {
> > +		skb->fclone = SKB_FCLONE_UNAVAILABLE;
> 
> Shouldn't that be SKB_FCLONE_FREE ?

Absolutely :(

I'll send a v2, thanks !

Re: [PATCH net] net: Revert "net: avoid one atomic operation in skb_clone()"

[Chris Mason]

On Fri, Nov 21, 2014 at 2:29 PM, Eric Dumazet <eric.dumazet@gmail.com> 
wrote:
> On Fri, 2014-11-21 at 19:05 +0100, Sabrina Dubroca wrote:
>>  Hello Eric,
>> 
>>  >
>>  > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>>  > index c16615bfb61e..be4c7deed971 100644
>>  > --- a/net/core/skbuff.c
>>  > +++ b/net/core/skbuff.c
>>  > @@ -552,20 +552,13 @@ static void kfree_skbmem(struct sk_buff 
>> *skb)
>>  >  	case SKB_FCLONE_CLONE:
>>  >  		fclones = container_of(skb, struct sk_buff_fclones, skb2);
>>  >
>>  > -		/* Warning : We must perform the atomic_dec_and_test() before
>>  > -		 * setting skb->fclone back to SKB_FCLONE_FREE, otherwise
>>  > -		 * skb_clone() could set clone_ref to 2 before our decrement.
>>  > -		 * Anyway, if we are going to free the structure, no need to
>>  > -		 * rewrite skb->fclone.
>>  > +		/* The clone portion is available for
>>  > +		 * fast-cloning again.
>>  >  		 */
>>  > -		if (atomic_dec_and_test(&fclones->fclone_ref)) {
>>  > +		skb->fclone = SKB_FCLONE_UNAVAILABLE;
>> 
>>  Shouldn't that be SKB_FCLONE_FREE ?
> 
> Absolutely :(
> 
> I'll send a v2, thanks !

v1 made it through the first pass here.  I'll switch to SKB_FCLONE_FREE 
and restart.

-chris

[PATCH v2 net] net: Revert "net: avoid one atomic operation in skb_clone()"

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Not sure what I was thinking, but doing anything after
releasing a refcount is suicidal or/and embarrassing.

By the time we set skb->fclone to SKB_FCLONE_FREE, another cpu
could have released last reference and freed whole skb.

We potentially corrupt memory or trap if CONFIG_DEBUG_PAGEALLOC is set.

Reported-by: Chris Mason <clm@fb.com>
Fixes: ce1a4ea3f1258 ("net: avoid one atomic operation in skb_clone()")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Sabrina Dubroca <sd@queasysnail.net>
---
v2: the revert went wrong, as SKB_FCLONE_UNAVAILABLE
    was later replaced by SKB_FCLONE_FREE, spotted by Sabrina Dubroca

 net/core/skbuff.c |   23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index c16615bfb61e..be4c7deed971 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -552,20 +552,13 @@ static void kfree_skbmem(struct sk_buff *skb)
 	case SKB_FCLONE_CLONE:
 		fclones = container_of(skb, struct sk_buff_fclones, skb2);
 
-		/* Warning : We must perform the atomic_dec_and_test() before
-		 * setting skb->fclone back to SKB_FCLONE_FREE, otherwise
-		 * skb_clone() could set clone_ref to 2 before our decrement.
-		 * Anyway, if we are going to free the structure, no need to
-		 * rewrite skb->fclone.
+		/* The clone portion is available for
+		 * fast-cloning again.
 		 */
-		if (atomic_dec_and_test(&fclones->fclone_ref)) {
+		skb->fclone = SKB_FCLONE_FREE;
+
+		if (atomic_dec_and_test(&fclones->fclone_ref))
 			kmem_cache_free(skbuff_fclone_cache, fclones);
-		} else {
-			/* The clone portion is available for
-			 * fast-cloning again.
-			 */
-			skb->fclone = SKB_FCLONE_FREE;
-		}
 		break;
 	}
 }
@@ -887,11 +880,7 @@ struct sk_buff *skb_clone(struct sk_buff *skb, gfp_t gfp_mask)
 	if (skb->fclone == SKB_FCLONE_ORIG &&
 	    n->fclone == SKB_FCLONE_FREE) {
 		n->fclone = SKB_FCLONE_CLONE;
-		/* As our fastclone was free, clone_ref must be 1 at this point.
-		 * We could use atomic_inc() here, but it is faster
-		 * to set the final value.
-		 */
-		atomic_set(&fclones->fclone_ref, 2);
+		atomic_inc(&fclones->fclone_ref);
 	} else {
 		if (skb_pfmemalloc(skb))
 			gfp_mask |= __GFP_MEMALLOC;

Re: [PATCH v2 net] net: Revert "net: avoid one atomic operation in skb_clone()"

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 21 Nov 2014 11:47:16 -0800

> From: Eric Dumazet <edumazet@google.com>
> 
> Not sure what I was thinking, but doing anything after
> releasing a refcount is suicidal or/and embarrassing.
> 
> By the time we set skb->fclone to SKB_FCLONE_FREE, another cpu
> could have released last reference and freed whole skb.
> 
> We potentially corrupt memory or trap if CONFIG_DEBUG_PAGEALLOC is set.
> 
> Reported-by: Chris Mason <clm@fb.com>
> Fixes: ce1a4ea3f1258 ("net: avoid one atomic operation in skb_clone()")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Sabrina Dubroca <sd@queasysnail.net>
> ---
> v2: the revert went wrong, as SKB_FCLONE_UNAVAILABLE
>     was later replaced by SKB_FCLONE_FREE, spotted by Sabrina Dubroca

Applied, thanks Eric.

Re: [PATCH v2 net] net: Revert "net: avoid one atomic operation in skb_clone()"

[Chris Mason]

On Fri, Nov 21, 2014 at 2:47 PM, Eric Dumazet <eric.dumazet@gmail.com> 
wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> v2: the revert went wrong, as SKB_FCLONE_UNAVAILABLE
>     was later replaced by SKB_FCLONE_FREE, spotted by Sabrina Dubroca
> 

v2 is happy here too.  I'll let it keep going over the weekend and add 
more memory pressure, but my crashes seem to be gone.

-chris