Message-ID: <20241029083658.1096492-1-elver@google.com>
Date: Tue, 29 Oct 2024 09:36:29 +0100
From: Marco Elver <elver@...gle.com>
To: elver@...gle.com, Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, 
	Will Deacon <will@...nel.org>, Waiman Long <longman@...hat.com>, Boqun Feng <boqun.feng@...il.com>
Cc: "Paul E. McKenney" <paulmck@...nel.org>, Thomas Gleixner <tglx@...utronix.de>, 
	Mark Rutland <mark.rutland@....com>, Dmitry Vyukov <dvyukov@...gle.com>, kasan-dev@...glegroups.com, 
	linux-kernel@...r.kernel.org, Alexander Potapenko <glider@...gle.com>
Subject: [PATCH] kcsan, seqlock: Support seqcount_latch_t

While fuzzing an arm64 kernel, Alexander Potapenko reported:

| BUG: KCSAN: data-race in ktime_get_mono_fast_ns / timekeeping_update
|
| write to 0xffffffc082e74248 of 56 bytes by interrupt on cpu 0:
|  update_fast_timekeeper kernel/time/timekeeping.c:430 [inline]
|  timekeeping_update+0x1d8/0x2d8 kernel/time/timekeeping.c:768
|  timekeeping_advance+0x9e8/0xb78 kernel/time/timekeeping.c:2344
|  update_wall_time+0x18/0x38 kernel/time/timekeeping.c:2360
|  tick_do_update_jiffies64+0x164/0x1b0 kernel/time/tick-sched.c:149
|  tick_nohz_handler+0xa4/0x2a8 kernel/time/tick-sched.c:232
|  __hrtimer_run_queues+0x198/0x33c kernel/time/hrtimer.c:1691
|  hrtimer_interrupt+0x16c/0x630 kernel/time/hrtimer.c:1817
|  timer_handler drivers/clocksource/arm_arch_timer.c:674 [inline]
|  arch_timer_handler_phys+0x60/0x74 drivers/clocksource/arm_arch_timer.c:692
|  handle_percpu_devid_irq+0xd8/0x1ec kernel/irq/chip.c:942
|  generic_handle_irq_desc include/linux/irqdesc.h:173 [inline]
|  handle_irq_desc kernel/irq/irqdesc.c:692 [inline]
|  generic_handle_domain_irq+0x5c/0x7c kernel/irq/irqdesc.c:748
|  gic_handle_irq+0x78/0x1b0 drivers/irqchip/irq-gic-v3.c:843
|  call_on_irq_stack+0x24/0x4c arch/arm64/kernel/entry.S:889
|  do_interrupt_handler+0x74/0xa8 arch/arm64/kernel/entry-common.c:310
|  __el1_irq arch/arm64/kernel/entry-common.c:536 [inline]
|  el1_interrupt+0x34/0x54 arch/arm64/kernel/entry-common.c:551
|  el1h_64_irq_handler+0x18/0x24 arch/arm64/kernel/entry-common.c:556
|  el1h_64_irq+0x64/0x68 arch/arm64/kernel/entry.S:594
|  __daif_local_irq_enable arch/arm64/include/asm/irqflags.h:26 [inline]
|  arch_local_irq_enable arch/arm64/include/asm/irqflags.h:48 [inline]
|  kvm_arch_vcpu_ioctl_run+0x8d4/0xf48 arch/arm64/kvm/arm.c:1259
|  kvm_vcpu_ioctl+0x650/0x894 virt/kvm/kvm_main.c:4487
|  __do_sys_ioctl fs/ioctl.c:51 [inline]
|  __se_sys_ioctl fs/ioctl.c:893 [inline]
|  __arm64_sys_ioctl+0xf8/0x170 fs/ioctl.c:893
|  [...]
|
| read to 0xffffffc082e74258 of 8 bytes by task 5260 on cpu 1:
|  __ktime_get_fast_ns kernel/time/timekeeping.c:372 [inline]
|  ktime_get_mono_fast_ns+0x88/0x174 kernel/time/timekeeping.c:489
|  init_srcu_struct_fields+0x40c/0x530 kernel/rcu/srcutree.c:263
|  init_srcu_struct+0x14/0x20 kernel/rcu/srcutree.c:311
|  kvm_dev_ioctl+0x304/0x908 virt/kvm/kvm_main.c:1185
|  __do_sys_ioctl fs/ioctl.c:51 [inline]
|  __se_sys_ioctl fs/ioctl.c:893 [inline]
|  __arm64_sys_ioctl+0xf8/0x170 fs/ioctl.c:893
|  [...]
|
| value changed: 0x000002f875d33266 -> 0x000002f877416866
|
| Reported by Kernel Concurrency Sanitizer on:
| CPU: 1 UID: 0 PID: 5260 Comm: syz.2.7483 Not tainted 6.12.0-rc3-dirty #78

This is a false positive data race between a seqcount latch writer and a
reader accessing stale data.

Unlike the regular seqlock interface, the seqcount_latch interface for
latch writers never has a well-defined critical section.

To support with KCSAN, optimistically declare that a fixed number of
memory accesses after raw_write_seqcount_latch() are "atomic". Latch
readers follow a similar pattern as the regular seqlock interface. This
effectively tells KCSAN that data races on accesses to seqcount_latch
protected data should be ignored.

Reviewing current raw_write_seqcount_latch() callers, the most common
patterns involve only few memory accesses, either a single plain C
assignment, or memcpy; therefore, the value of 8 memory accesses after
raw_write_seqcount_latch() is chosen to (a) avoid most false positives,
and (b) avoid excessive number of false negatives (due to inadvertently
declaring most accesses in the proximity of update_fast_timekeeper() as
"atomic").

Reported-by: Alexander Potapenko <glider@...gle.com>
Tested-by: Alexander Potapenko <glider@...gle.com>
Fixes: 88ecd153be95 ("seqlock, kcsan: Add annotations for KCSAN")
Signed-off-by: Marco Elver <elver@...gle.com>
---
 include/linux/seqlock.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
index fffeb754880f..e24cf144276e 100644
--- a/include/linux/seqlock.h
+++ b/include/linux/seqlock.h
@@ -614,6 +614,7 @@ typedef struct {
  */
 static __always_inline unsigned raw_read_seqcount_latch(const seqcount_latch_t *s)
 {
+	kcsan_atomic_next(KCSAN_SEQLOCK_REGION_MAX);
 	/*
 	 * Pairs with the first smp_wmb() in raw_write_seqcount_latch().
 	 * Due to the dependent load, a full smp_rmb() is not needed.
@@ -631,6 +632,7 @@ static __always_inline unsigned raw_read_seqcount_latch(const seqcount_latch_t *
 static __always_inline int
 raw_read_seqcount_latch_retry(const seqcount_latch_t *s, unsigned start)
 {
+	kcsan_atomic_next(0);
 	smp_rmb();
 	return unlikely(READ_ONCE(s->seqcount.sequence) != start);
 }
@@ -721,6 +723,13 @@ static inline void raw_write_seqcount_latch(seqcount_latch_t *s)
 	smp_wmb();	/* prior stores before incrementing "sequence" */
 	s->seqcount.sequence++;
 	smp_wmb();      /* increment "sequence" before following stores */
+
+	/*
+	 * Latch writers do not have a well-defined critical section, but to
+	 * avoid most false positives, at the cost of false negatives, assume
+	 * the next few memory accesses belong to the latch writer.
+	 */
+	kcsan_atomic_next(8);
 }
 
 #define __SEQLOCK_UNLOCKED(lockname)					\
-- 
2.47.0.163.g1226f6d8fa-goog

Powered by blists - more mailing lists