[RFC] net: fix data race on sk_buff after re-cloning

[Dmitry Vyukov]

KernelThreadSanitizer (KTSAN) reported the following race (on 4.2 rc2):

ThreadSanitizer: data-race in __copy_skb_header

Write at 0xffff8800bb158f48 of size 8 by thread 3146 on CPU 5:
[<ffffffff81b699fe>] __copy_skb_header+0xee/0x1d0 net/core/skbuff.c:765
[<ffffffff81b69b3c>] __skb_clone+0x5c/0x320 net/core/skbuff.c:820
[<ffffffff81b6c750>] skb_clone+0xd0/0x130 net/core/skbuff.c:962
[<ffffffff81c4a295>] tcp_transmit_skb+0xb5/0x1750 net/ipv4/tcp_output.c:932
[<ffffffff81c4f564>] __tcp_retransmit_skb+0x244/0xb10 net/ipv4/tcp_output.c:2638
[<ffffffff81c501fb>] tcp_retransmit_skb+0x2b/0x240 net/ipv4/tcp_output.c:2655
[<ffffffff81c53229>] tcp_retransmit_timer+0x579/0xb70 net/ipv4/tcp_timer.c:433
[<ffffffff81c53929>] tcp_write_timer_handler+0x109/0x320 net/ipv4/tcp_timer.c:514
[<ffffffff81c53c00>] tcp_write_timer+0xc0/0xe0 net/ipv4/tcp_timer.c:532
[<ffffffff8110e74c>] call_timer_fn+0x4c/0x1b0 kernel/time/timer.c:1155
[< inline >] __run_timers kernel/time/timer.c:1231
[<ffffffff8110f433>] run_timer_softirq+0x313/0x500 kernel/time/timer.c:1414
[<ffffffff81091c1e>] __do_softirq+0xbe/0x2f0 kernel/softirq.c:273
[<ffffffff81ee4cca>] apic_timer_interrupt+0x8a/0xa0 arch/x86/entry/entry_64.S:790

Previous read at 0xffff8800bb158f48 of size 8 by thread 3168 on CPU 0:
[<ffffffff81b693ab>] skb_release_head_state+0x4b/0x120 net/core/skbuff.c:640
[<ffffffff81b6ac6d>] skb_release_all+0x1d/0x50 net/core/skbuff.c:657
[< inline >] __kfree_skb net/core/skbuff.c:673
[<ffffffff81b6af20>] consume_skb+0x60/0x100 net/core/skbuff.c:746
[<ffffffff81b8a69d>] __dev_kfree_skb_any+0x4d/0x60 net/core/dev.c:2312
[< inline >] dev_kfree_skb_any include/linux/netdevice.h:2933
[<ffffffff8194a703>] e1000_unmap_and_free_tx_resource.isra.42+0xd3/0x120 drivers/net/ethernet/intel/e1000/e1000_main.c:1973
[< inline >] e1000_clean_tx_irq drivers/net/ethernet/intel/e1000/e1000_main.c:3881
[<ffffffff8194a99d>] e1000_clean+0x24d/0x11e0 drivers/net/ethernet/intel/e1000/e1000_main.c:3818
[< inline >] napi_poll net/core/dev.c:4744
[<ffffffff81b8df79>] net_rx_action+0x489/0x690 net/core/dev.c:4809
[<ffffffff81091c1e>] __do_softirq+0xbe/0x2f0 kernel/softirq.c:273
[<ffffffff81ee4cca>] apic_timer_interrupt+0x8a/0xa0 arch/x86/entry/entry_64.S:790

Mutexes locked by thread 3146:
Mutex 436586 is locked here:
[< inline >] __raw_spin_lock include/linux/spinlock_api_smp.h:158
[<ffffffff81ee37c0>] _raw_spin_lock+0x50/0x70 kernel/locking/spinlock.c:151
[< inline >] spin_lock include/linux/spinlock.h:312
[<ffffffff81c53b65>] tcp_write_timer+0x25/0xe0 net/ipv4/tcp_timer.c:530
[<ffffffff8110e74c>] call_timer_fn+0x4c/0x1b0 kernel/time/timer.c:1155
[< inline >] __run_timers kernel/time/timer.c:1231
[<ffffffff8110f433>] run_timer_softirq+0x313/0x500 kernel/time/timer.c:1414
[<ffffffff81091c1e>] __do_softirq+0xbe/0x2f0 kernel/softirq.c:273
[<ffffffff81ee4cca>] apic_timer_interrupt+0x8a/0xa0 arch/x86/entry/entry_64.S:790

The only way I can see it happens is as follows:
- sk_buff_fclones is allocated
- then it is cloned which returns fclones->skb2
- then fclones->skb2 is freed, which drops fclones->fclone_ref to 1
- then the original skb is cloned again
- at this point skb_clone sees that fclones->fclone_ref = 1
and returns fclones->skb2 again
Now initialization of fclones->skb2 races with the previous use,
because refcounting lacks proper memory barriers.

I am looking at skb code for the first time, so I can't conclude
whether such scenario is possible or not. But refcount at least in
kfree_skbmem() looks broken. For example, kfree_skb() properly
inserts rmb after the fast-path check:

if (likely(atomic_read(&skb->users) == 1))
smp_rmb();

The patch contains a proposed fix.
If it looks good to you and the scenario looks sane,
then I will update the description and resend it.
---
net/core/skbuff.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index dad4dd3..4c89bac 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -618,8 +618,9 @@ static void kfree_skbmem(struct sk_buff *skb)
/* We usually free the clone (TX completion) before original skb
* This test would have no chance to be true for the clone,
* while here, branch prediction will be good.
+ * Paired with atomic_dec_and_test() below.
*/
- if (atomic_read(&fclones->fclone_ref) == 1)
+ if (atomic_read_acquire(&fclones->fclone_ref) == 1)
goto fastpath;
break;

@@ -944,7 +945,8 @@ struct sk_buff *skb_clone(struct sk_buff *skb, gfp_t gfp_mask)
return NULL;

if (skb->fclone == SKB_FCLONE_ORIG &&
- atomic_read(&fclones->fclone_ref) == 1) {
+ /* Paired with atomic_dec_and_test() in kfree_skbmem(). */
+ atomic_read_acquire(&fclones->fclone_ref) == 1) {
n = &fclones->skb2;
atomic_set(&fclones->fclone_ref, 2);
} else {
--
2.6.0.rc0.131.gf624c3d