Commit bbf3f8cb authored by Mikulas Patocka's avatar Mikulas Patocka Committed by Mike Snitzer

dm stats: fix possible counter corruption on 32-bit systems

There was a deliberate race condition in dm_stat_for_entry() to avoid the
overhead of disabling and enabling interrupts.  The race could result in
some events not being counted on 64-bit architectures.

However, on 32-bit architectures, operations on long long variables are
not atomic, so the race condition could cause the counter to jump by 2^32.
Such jumps could be disruptive, so we need to do proper locking on 32-bit
architectures.
Signed-off-by: default avatarMikulas Patocka <mpatocka@redhat.com>
Cc: Alasdair G. Kergon <agk@redhat.com>
Signed-off-by: default avatarMike Snitzer <snitzer@redhat.com>
parent cc9d3c38
...@@ -451,19 +451,26 @@ static void dm_stat_for_entry(struct dm_stat *s, size_t entry, ...@@ -451,19 +451,26 @@ static void dm_stat_for_entry(struct dm_stat *s, size_t entry,
struct dm_stat_percpu *p; struct dm_stat_percpu *p;
/* /*
* For strict correctness we should use local_irq_disable/enable * For strict correctness we should use local_irq_save/restore
* instead of preempt_disable/enable. * instead of preempt_disable/enable.
* *
* This is racy if the driver finishes bios from non-interrupt * preempt_disable/enable is racy if the driver finishes bios
* context as well as from interrupt context or from more different * from non-interrupt context as well as from interrupt context
* interrupts. * or from more different interrupts.
* *
* However, the race only results in not counting some events, * On 64-bit architectures the race only results in not counting some
* so it is acceptable. * events, so it is acceptable. On 32-bit architectures the race could
* cause the counter going off by 2^32, so we need to do proper locking
* there.
* *
* part_stat_lock()/part_stat_unlock() have this race too. * part_stat_lock()/part_stat_unlock() have this race too.
*/ */
#if BITS_PER_LONG == 32
unsigned long flags;
local_irq_save(flags);
#else
preempt_disable(); preempt_disable();
#endif
p = &s->stat_percpu[smp_processor_id()][entry]; p = &s->stat_percpu[smp_processor_id()][entry];
if (!end) { if (!end) {
...@@ -478,7 +485,11 @@ static void dm_stat_for_entry(struct dm_stat *s, size_t entry, ...@@ -478,7 +485,11 @@ static void dm_stat_for_entry(struct dm_stat *s, size_t entry,
p->ticks[idx] += duration; p->ticks[idx] += duration;
} }
#if BITS_PER_LONG == 32
local_irq_restore(flags);
#else
preempt_enable(); preempt_enable();
#endif
} }
static void __dm_stat_bio(struct dm_stat *s, unsigned long bi_rw, static void __dm_stat_bio(struct dm_stat *s, unsigned long bi_rw,
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment