Date:	Fri, 18 Sep 2015 15:54:23 +0200
From:	Dmitry Vyukov <dvyukov@...gle.com>
To:	john.stultz@...aro.org, tglx@...utronix.de,
	linux-kernel@...r.kernel.org
Cc:	andreyknvl@...gle.com, glider@...gle.com, kcc@...gle.com,
	ktsan@...glegroups.com, Dmitry Vyukov <dvyukov@...gle.com>
Subject: [PATCH] kernel/time: fix data race in timer_stats_account_timer

timer_stats_account_timer() reads timer->start_site,
then checks it for NULL and then re-reads it again.
While timer_stats_timer_clear_start_info() can concurrently
reset timer->start_site to NULL. This should not lead to
crashes, but can double number of entries in timer stats
as start_site is used during comparison, the doubled entries
will have unuseful NULL start_site.

Read timer->start_site only once in timer_stats_account_timer().

The data race was found with KernelThreadSanitizer (KTSAN).

Signed-off-by: Dmitry Vyukov <dvyukov@...gle.com>
---

The race report for the record (on 4.2 rc2):

ThreadSanitizer: data-race in run_timer_softirq

Read at 0xffff8804834f0d08 of size 8 by thread 1011 on CPU 8:
 [<     inline     >] timer_stats_account_timer kernel/time/timer.c:464
 [<     inline     >] __run_timers kernel/time/timer.c:1220
 [<ffffffff8110f2dc>] run_timer_softirq+0x1ac/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 write at 0xffff8804834f0d08 of size 8 by thread 1012 on CPU 7:
 [<     inline     >] timer_stats_timer_clear_start_info include/linux/timer.h:208
 [<ffffffff8110e585>] del_timer+0x25/0x80 kernel/time/timer.c:1005
 [<     inline     >] cfq_del_timer block/cfq-iosched.c:2497
 [<ffffffff81541552>] __cfq_slice_expired+0x3d2/0x990 block/cfq-iosched.c:2537
 [<     inline     >] cfq_slice_expired block/cfq-iosched.c:2583
 [<     inline     >] cfq_preempt_queue block/cfq-iosched.c:3935
 [<     inline     >] cfq_rq_enqueued block/cfq-iosched.c:4005
 [<ffffffff81545aee>] cfq_insert_request+0x95e/0xc70 block/cfq-iosched.c:4023
 [<ffffffff81509ab3>] __elv_add_request+0x293/0x4e0 block/elevator.c:659
 [<ffffffff81516387>] blk_flush_plug_list+0x3a7/0x420 block/blk-core.c:3204
 [<ffffffff81516974>] blk_finish_plug+0x54/0x80 block/blk-core.c:3222
 [<     inline     >] read_pages mm/readahead.c:139
 [<ffffffff811e0e8b>] __do_page_cache_readahead+0x29b/0x3a0 mm/readahead.c:200
 [<ffffffff811e10e2>] ondemand_readahead+0x152/0x370 mm/readahead.c:454
 [<ffffffff811e15c6>] page_cache_sync_readahead+0x66/0xa0 mm/readahead.c:504
 [<     inline     >] do_generic_file_read mm/filemap.c:1516
 [<ffffffff811ce712>] generic_file_read_iter+0x712/0x880 mm/filemap.c:1759
 [<     inline     >] new_sync_read fs/read_write.c:422
 [<ffffffff8125f530>] __vfs_read+0x1a0/0x1e0 fs/read_write.c:434
 [<ffffffff81260337>] vfs_read+0xb7/0x1e0 fs/read_write.c:454
 [<ffffffff81269429>] kernel_read+0x79/0xb0 fs/exec.c:821
 [<ffffffff81269a2d>] prepare_binprm+0x15d/0x2b0 fs/exec.c:1347
 [<ffffffff8126c590>] do_execveat_common.isra.36+0x820/0xb40 fs/exec.c:1567
 [<     inline     >] do_execve fs/exec.c:1628
 [<     inline     >] SYSC_execve fs/exec.c:1709
 [<ffffffff8126cdc6>] SyS_execve+0x46/0x60 fs/exec.c:1704
 [<ffffffff81ee4145>] return_from_execve+0x0/0x23 arch/x86/entry/entry_64.S:428

Mutexes locked by thread 1011:
Mutex 2498 is locked here:
 [<     inline     >] __raw_spin_lock_irq include/linux/spinlock_api_smp.h:140
 [<ffffffff81ee375c>] _raw_spin_lock_irq+0x5c/0x80 kernel/locking/spinlock.c:167
 [<     inline     >] spin_lock_irq include/linux/spinlock.h:342
 [<     inline     >] __run_timers kernel/time/timer.c:1186
 [<ffffffff8110f198>] run_timer_softirq+0x68/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

Mutexes locked by thread 1012:
Mutex 128390 is locked here:
 [<ffffffff81edf7c2>] mutex_lock_interruptible+0x62/0xa0 kernel/locking/mutex.c:805
 [<ffffffff8126bd0f>] prepare_bprm_creds+0x4f/0xb0 fs/exec.c:1172
 [<ffffffff8126beaf>] do_execveat_common.isra.36+0x13f/0xb40 fs/exec.c:1517
 [<     inline     >] do_execve fs/exec.c:1628
 [<     inline     >] SYSC_execve fs/exec.c:1709
 [<ffffffff8126cdc6>] SyS_execve+0x46/0x60 fs/exec.c:1704
 [<ffffffff81ee4145>] return_from_execve+0x0/0x23 arch/x86/entry/entry_64.S:428

Mutex 106756 is locked here:
 [<     inline     >] __raw_spin_lock include/linux/spinlock_api_smp.h:158
 [<ffffffff81ee37d0>] _raw_spin_lock+0x50/0x70 kernel/locking/spinlock.c:151
 [<     inline     >] spin_lock include/linux/spinlock.h:312
 [<ffffffff8151633f>] blk_flush_plug_list+0x35f/0x420 block/blk-core.c:3187
 [<ffffffff81516974>] blk_finish_plug+0x54/0x80 block/blk-core.c:3222
 [<     inline     >] read_pages mm/readahead.c:139
 [<ffffffff811e0e8b>] __do_page_cache_readahead+0x29b/0x3a0 mm/readahead.c:200
 [<ffffffff811e10e2>] ondemand_readahead+0x152/0x370 mm/readahead.c:454
 [<ffffffff811e15c6>] page_cache_sync_readahead+0x66/0xa0 mm/readahead.c:504
 [<     inline     >] do_generic_file_read mm/filemap.c:1516
 [<ffffffff811ce712>] generic_file_read_iter+0x712/0x880 mm/filemap.c:1759
 [<     inline     >] new_sync_read fs/read_write.c:422
 [<ffffffff8125f530>] __vfs_read+0x1a0/0x1e0 fs/read_write.c:434
 [<ffffffff81260337>] vfs_read+0xb7/0x1e0 fs/read_write.c:454
 [<ffffffff81269429>] kernel_read+0x79/0xb0 fs/exec.c:821
 [<ffffffff81269a2d>] prepare_binprm+0x15d/0x2b0 fs/exec.c:1347
 [<ffffffff8126c590>] do_execveat_common.isra.36+0x820/0xb40 fs/exec.c:1567
 [<     inline     >] do_execve fs/exec.c:1628
 [<     inline     >] SYSC_execve fs/exec.c:1709
 [<ffffffff8126cdc6>] SyS_execve+0x46/0x60 fs/exec.c:1704
 [<ffffffff81ee4145>] return_from_execve+0x0/0x23 arch/x86/entry/entry_64.S:428
---
 kernel/time/timer.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 5e097fa..96f7d1f 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -461,10 +461,15 @@ void __timer_stats_timer_set_start_info(struct timer_list *timer, void *addr)
 
 static void timer_stats_account_timer(struct timer_list *timer)
 {
-	if (likely(!timer->start_site))
+	void *site;
+
+	/* start_site can be concurrently reset by
+	   timer_stats_timer_clear_start_info() */
+	site = READ_ONCE(timer->start_site);
+	if (likely(!site))
 		return;
 
-	timer_stats_update_stats(timer, timer->start_pid, timer->start_site,
+	timer_stats_update_stats(timer, timer->start_pid, site,
 				 timer->function, timer->start_comm,
 				 timer->flags);
 }
-- 
2.6.0.rc0.131.gf624c3d

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists