2020-02-25 16:28:36

by Qian Cai

Subject: [PATCH] char/random: fix data races at timer_rand_state

Fields in "struct timer_rand_state" could be accessed concurrently.
Lockless plain reads and writes result in data races. Fix them by adding
pairs of READ|WRITE_ONCE(). The data races were reported by KCSAN,

BUG: KCSAN: data-race in add_timer_randomness / add_timer_randomness

write to 0xffff9f320a0a01d0 of 8 bytes by interrupt on cpu 22:
add_timer_randomness+0x100/0x190
add_timer_randomness at drivers/char/random.c:1152
add_disk_randomness+0x85/0x280
scsi_end_request+0x43a/0x4a0
scsi_io_completion+0xb7/0x7e0
scsi_finish_command+0x1ed/0x2a0
scsi_softirq_done+0x1c9/0x1d0
blk_done_softirq+0x181/0x1d0
__do_softirq+0xd9/0x57c
irq_exit+0xa2/0xc0
do_IRQ+0x8b/0x190
ret_from_intr+0x0/0x42
cpuidle_enter_state+0x15e/0x980
cpuidle_enter+0x69/0xc0
call_cpuidle+0x23/0x40
do_idle+0x248/0x280
cpu_startup_entry+0x1d/0x1f
start_secondary+0x1b2/0x230
secondary_startup_64+0xb6/0xc0

no locks held by swapper/22/0.
irq event stamp: 32871382
_raw_spin_unlock_irqrestore+0x53/0x60
_raw_spin_lock_irqsave+0x21/0x60
_local_bh_enable+0x21/0x30
irq_exit+0xa2/0xc0

read to 0xffff9f320a0a01d0 of 8 bytes by interrupt on cpu 2:
add_timer_randomness+0xe8/0x190
add_disk_randomness+0x85/0x280
scsi_end_request+0x43a/0x4a0
scsi_io_completion+0xb7/0x7e0
scsi_finish_command+0x1ed/0x2a0
scsi_softirq_done+0x1c9/0x1d0
blk_done_softirq+0x181/0x1d0
__do_softirq+0xd9/0x57c
irq_exit+0xa2/0xc0
do_IRQ+0x8b/0x190
ret_from_intr+0x0/0x42
cpuidle_enter_state+0x15e/0x980
cpuidle_enter+0x69/0xc0
call_cpuidle+0x23/0x40
do_idle+0x248/0x280
cpu_startup_entry+0x1d/0x1f
start_secondary+0x1b2/0x230
secondary_startup_64+0xb6/0xc0

no locks held by swapper/2/0.
irq event stamp: 37846304
_raw_spin_unlock_irqrestore+0x53/0x60
_raw_spin_lock_irqsave+0x21/0x60
_local_bh_enable+0x21/0x30
irq_exit+0xa2/0xc0

Reported by Kernel Concurrency Sanitizer on:
Hardware name: HP ProLiant BL660c Gen9, BIOS I38 10/17/2018

Signed-off-by: Qian Cai <[email protected]>
---
drivers/char/random.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index c7f9584de2c8..85cabb17b23f 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1142,14 +1142,14 @@ static void add_timer_randomness(struct timer_rand_state *state, unsigned num)
* We take into account the first, second and third-order deltas
* in order to make our estimate.
*/
- delta = sample.jiffies - state->last_time;
- state->last_time = sample.jiffies;
+ delta = sample.jiffies - READ_ONCE(state->last_time);
+ WRITE_ONCE(state->last_time, sample.jiffies);

- delta2 = delta - state->last_delta;
- state->last_delta = delta;
+ delta2 = delta - READ_ONCE(state->last_delta);
+ WRITE_ONCE(state->last_delta, delta);

- delta3 = delta2 - state->last_delta2;
- state->last_delta2 = delta2;
+ delta3 = delta2 - READ_ONCE(state->last_delta2);
+ WRITE_ONCE(state->last_delta2, delta2);

if (delta < 0)
delta = -delta;
--
1.8.3.1

2020-02-28 04:12:44

by Theodore Ts'o

Subject: Re: [PATCH] char/random: fix data races at timer_rand_state

On Tue, Feb 25, 2020 at 11:27:04AM -0500, Qian Cai wrote:
> Fields in "struct timer_rand_state" could be accessed concurrently.
> Lockless plain reads and writes result in data races. Fix them by adding
> pairs of READ|WRITE_ONCE(). The data races were reported by KCSAN,
>
> BUG: KCSAN: data-race in add_timer_randomness / add_timer_randomness
>
> write to 0xffff9f320a0a01d0 of 8 bytes by interrupt on cpu 22:
> add_timer_randomness+0x100/0x190
> add_timer_randomness at drivers/char/random.c:1152
> add_disk_randomness+0x85/0x280
> scsi_end_request+0x43a/0x4a0
> scsi_io_completion+0xb7/0x7e0
> scsi_finish_command+0x1ed/0x2a0
> scsi_softirq_done+0x1c9/0x1d0
> blk_done_softirq+0x181/0x1d0
> __do_softirq+0xd9/0x57c
> irq_exit+0xa2/0xc0
> do_IRQ+0x8b/0x190
> ret_from_intr+0x0/0x42
> cpuidle_enter_state+0x15e/0x980
> cpuidle_enter+0x69/0xc0
> call_cpuidle+0x23/0x40
> do_idle+0x248/0x280
> cpu_startup_entry+0x1d/0x1f
> start_secondary+0x1b2/0x230
> secondary_startup_64+0xb6/0xc0
>
> no locks held by swapper/22/0.
> irq event stamp: 32871382
> _raw_spin_unlock_irqrestore+0x53/0x60
> _raw_spin_lock_irqsave+0x21/0x60
> _local_bh_enable+0x21/0x30
> irq_exit+0xa2/0xc0
>
> read to 0xffff9f320a0a01d0 of 8 bytes by interrupt on cpu 2:
> add_timer_randomness+0xe8/0x190
> add_disk_randomness+0x85/0x280
> scsi_end_request+0x43a/0x4a0
> scsi_io_completion+0xb7/0x7e0
> scsi_finish_command+0x1ed/0x2a0
> scsi_softirq_done+0x1c9/0x1d0
> blk_done_softirq+0x181/0x1d0
> __do_softirq+0xd9/0x57c
> irq_exit+0xa2/0xc0
> do_IRQ+0x8b/0x190
> ret_from_intr+0x0/0x42
> cpuidle_enter_state+0x15e/0x980
> cpuidle_enter+0x69/0xc0
> call_cpuidle+0x23/0x40
> do_idle+0x248/0x280
> cpu_startup_entry+0x1d/0x1f
> start_secondary+0x1b2/0x230
> secondary_startup_64+0xb6/0xc0
>
> no locks held by swapper/2/0.
> irq event stamp: 37846304
> _raw_spin_unlock_irqrestore+0x53/0x60
> _raw_spin_lock_irqsave+0x21/0x60
> _local_bh_enable+0x21/0x30
> irq_exit+0xa2/0xc0
>
> Reported by Kernel Concurrency Sanitizer on:
> Hardware name: HP ProLiant BL660c Gen9, BIOS I38 10/17/2018
>
> Signed-off-by: Qian Cai <[email protected]>

Thanks, applied.

- Ted