[PATCH 0/3] block/accounting: fix unlocked latency histogram reconfiguration races

[PATCH 0/3] block/accounting: fix unlocked latency histogram reconfiguration races

[Denis V. Lunev]

block_latency_histogram_set() and block_latency_histograms_clear()
replace BlockLatencyHistogram's nbins/boundaries/bins without taking
stats->lock, while block_account_one_io() reads those same fields
under that lock from whatever iothread completes the I/O. A histogram
reconfiguration (block-latency-histogram-set QMP command, monitor
thread) racing an in-flight completion can therefore observe those
fields torn, hitting assert(pos != NULL) in
block_latency_histogram_account(), or corrupting the heap outright.
This showed up as a qemu-kvm SIGABRT on a customer's virtio-blk guest.

Regression test is added for illustrative purpose but I am unsure that
it is viable long term. Feel free to drop.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Hanna Reitz <hreitz@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Denis V. Lunev (3):
  block/accounting: take stats->lock in latency histogram setters
  block/qapi: take stats->lock when reading BlockAcctStats for
    query-blockstats
  tests/unit: add reproducer for BlockAcctStats histogram locking race

 block/accounting.c                 |  34 +++++----
 block/qapi.c                       | 102 +++++++++++++++-----------
 tests/unit/meson.build             |   1 +
 tests/unit/test-block-accounting.c | 114 +++++++++++++++++++++++++++++
 4 files changed, 194 insertions(+), 57 deletions(-)
 create mode 100644 tests/unit/test-block-accounting.c

-- 
2.53.0

[PATCH 1/3] block/accounting: take stats->lock in latency histogram setters

[Denis V. Lunev]

block_latency_histogram_set() and block_latency_histograms_clear()
replace BlockLatencyHistogram's nbins/boundaries/bins without taking
stats->lock, while block_account_one_io() reads those same fields
under that lock from whatever iothread completes the I/O. The result
is usual use-after-free and qemu crash.

Take stats->lock in both setters, matching the lock already held by
the reader.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Hanna Reitz <hreitz@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 block/accounting.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/block/accounting.c b/block/accounting.c
index f00fe99740..c277b796eb 100644
--- a/block/accounting.c
+++ b/block/accounting.c
@@ -194,17 +194,19 @@ int block_latency_histogram_set(BlockAcctStats *stats, enum BlockAcctType type,
         return -EINVAL;
     }
 
-    hist->nbins = new_nbins;
-    g_free(hist->boundaries);
-    hist->boundaries = g_new(uint64_t, hist->nbins - 1);
-    for (entry = boundaries, ptr = hist->boundaries; entry;
-         entry = entry->next, ptr++)
-    {
-        *ptr = entry->value;
-    }
+    WITH_QEMU_LOCK_GUARD(&stats->lock) {
+        hist->nbins = new_nbins;
+        g_free(hist->boundaries);
+        hist->boundaries = g_new(uint64_t, hist->nbins - 1);
+        for (entry = boundaries, ptr = hist->boundaries; entry;
+             entry = entry->next, ptr++)
+        {
+            *ptr = entry->value;
+        }
 
-    g_free(hist->bins);
-    hist->bins = g_new0(uint64_t, hist->nbins);
+        g_free(hist->bins);
+        hist->bins = g_new0(uint64_t, hist->nbins);
+    }
 
     return 0;
 }
@@ -213,11 +215,13 @@ void block_latency_histograms_clear(BlockAcctStats *stats)
 {
     int i;
 
-    for (i = 0; i < BLOCK_MAX_IOTYPE; i++) {
-        BlockLatencyHistogram *hist = &stats->latency_histogram[i];
-        g_free(hist->bins);
-        g_free(hist->boundaries);
-        memset(hist, 0, sizeof(*hist));
+    WITH_QEMU_LOCK_GUARD(&stats->lock) {
+        for (i = 0; i < BLOCK_MAX_IOTYPE; i++) {
+            BlockLatencyHistogram *hist = &stats->latency_histogram[i];
+            g_free(hist->bins);
+            g_free(hist->boundaries);
+            memset(hist, 0, sizeof(*hist));
+        }
     }
 }
 
-- 
2.53.0

[PATCH 2/3] block/qapi: take stats->lock when reading BlockAcctStats for query-blockstats

[Denis V. Lunev]

bdrv_query_blk_stats() reads nr_bytes[], nr_ops[], failed_ops[],
invalid_ops[], merged[], total_time_ns[], last_access_time_ns, and
the latency histogram bins directly off BlockAcctStats, without
taking stats->lock. block_account_one_io() updates the same fields
under that lock from whatever iothread completes the I/O, so this is
an unsynchronized read of concurrently-updated state.

This is a narrower issue than the one fixed for the histogram
setters. query-blockstats and block-latency-histogram-set are both
plain (non-allow-oob) QMP commands, so they always run serialized on
the monitor thread and can never execute concurrently with each
other.

Take stats->lock around the read for consistency.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Hanna Reitz <hreitz@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 block/qapi.c | 102 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 60 insertions(+), 42 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index eabfbfc258..c3f20832b7 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -535,49 +535,67 @@ static void bdrv_query_blk_stats(BlockDeviceStats *ds, BlockBackend *blk)
     BlockAcctTimedStats *ts = NULL;
     BlockLatencyHistogram *hgram;
 
-    ds->rd_bytes = stats->nr_bytes[BLOCK_ACCT_READ];
-    ds->wr_bytes = stats->nr_bytes[BLOCK_ACCT_WRITE];
-    ds->zone_append_bytes = stats->nr_bytes[BLOCK_ACCT_ZONE_APPEND];
-    ds->unmap_bytes = stats->nr_bytes[BLOCK_ACCT_UNMAP];
-    ds->rd_operations = stats->nr_ops[BLOCK_ACCT_READ];
-    ds->wr_operations = stats->nr_ops[BLOCK_ACCT_WRITE];
-    ds->zone_append_operations = stats->nr_ops[BLOCK_ACCT_ZONE_APPEND];
-    ds->unmap_operations = stats->nr_ops[BLOCK_ACCT_UNMAP];
-
-    ds->failed_rd_operations = stats->failed_ops[BLOCK_ACCT_READ];
-    ds->failed_wr_operations = stats->failed_ops[BLOCK_ACCT_WRITE];
-    ds->failed_zone_append_operations =
-        stats->failed_ops[BLOCK_ACCT_ZONE_APPEND];
-    ds->failed_flush_operations = stats->failed_ops[BLOCK_ACCT_FLUSH];
-    ds->failed_unmap_operations = stats->failed_ops[BLOCK_ACCT_UNMAP];
-
-    ds->invalid_rd_operations = stats->invalid_ops[BLOCK_ACCT_READ];
-    ds->invalid_wr_operations = stats->invalid_ops[BLOCK_ACCT_WRITE];
-    ds->invalid_zone_append_operations =
-        stats->invalid_ops[BLOCK_ACCT_ZONE_APPEND];
-    ds->invalid_flush_operations =
-        stats->invalid_ops[BLOCK_ACCT_FLUSH];
-    ds->invalid_unmap_operations = stats->invalid_ops[BLOCK_ACCT_UNMAP];
-
-    ds->rd_merged = stats->merged[BLOCK_ACCT_READ];
-    ds->wr_merged = stats->merged[BLOCK_ACCT_WRITE];
-    ds->zone_append_merged = stats->merged[BLOCK_ACCT_ZONE_APPEND];
-    ds->unmap_merged = stats->merged[BLOCK_ACCT_UNMAP];
-    ds->flush_operations = stats->nr_ops[BLOCK_ACCT_FLUSH];
-    ds->wr_total_time_ns = stats->total_time_ns[BLOCK_ACCT_WRITE];
-    ds->zone_append_total_time_ns =
-        stats->total_time_ns[BLOCK_ACCT_ZONE_APPEND];
-    ds->rd_total_time_ns = stats->total_time_ns[BLOCK_ACCT_READ];
-    ds->flush_total_time_ns = stats->total_time_ns[BLOCK_ACCT_FLUSH];
-    ds->unmap_total_time_ns = stats->total_time_ns[BLOCK_ACCT_UNMAP];
-
-    ds->has_idle_time_ns = stats->last_access_time_ns > 0;
-    if (ds->has_idle_time_ns) {
-        ds->idle_time_ns = block_acct_idle_time_ns(stats);
-    }
+    /*
+     * nr_bytes[] etc. are mutated under stats->lock by block_account_one_io()
+     * from an iothread, and latency_histogram[].{nbins,boundaries,bins} can
+     * be freed and reallocated by a concurrent block_latency_histogram_set()
+     * on the monitor thread, so every read here needs the same lock.
+     */
+    WITH_QEMU_LOCK_GUARD(&stats->lock) {
+        ds->rd_bytes = stats->nr_bytes[BLOCK_ACCT_READ];
+        ds->wr_bytes = stats->nr_bytes[BLOCK_ACCT_WRITE];
+        ds->zone_append_bytes = stats->nr_bytes[BLOCK_ACCT_ZONE_APPEND];
+        ds->unmap_bytes = stats->nr_bytes[BLOCK_ACCT_UNMAP];
+        ds->rd_operations = stats->nr_ops[BLOCK_ACCT_READ];
+        ds->wr_operations = stats->nr_ops[BLOCK_ACCT_WRITE];
+        ds->zone_append_operations = stats->nr_ops[BLOCK_ACCT_ZONE_APPEND];
+        ds->unmap_operations = stats->nr_ops[BLOCK_ACCT_UNMAP];
+
+        ds->failed_rd_operations = stats->failed_ops[BLOCK_ACCT_READ];
+        ds->failed_wr_operations = stats->failed_ops[BLOCK_ACCT_WRITE];
+        ds->failed_zone_append_operations =
+            stats->failed_ops[BLOCK_ACCT_ZONE_APPEND];
+        ds->failed_flush_operations = stats->failed_ops[BLOCK_ACCT_FLUSH];
+        ds->failed_unmap_operations = stats->failed_ops[BLOCK_ACCT_UNMAP];
+
+        ds->invalid_rd_operations = stats->invalid_ops[BLOCK_ACCT_READ];
+        ds->invalid_wr_operations = stats->invalid_ops[BLOCK_ACCT_WRITE];
+        ds->invalid_zone_append_operations =
+            stats->invalid_ops[BLOCK_ACCT_ZONE_APPEND];
+        ds->invalid_flush_operations =
+            stats->invalid_ops[BLOCK_ACCT_FLUSH];
+        ds->invalid_unmap_operations = stats->invalid_ops[BLOCK_ACCT_UNMAP];
+
+        ds->rd_merged = stats->merged[BLOCK_ACCT_READ];
+        ds->wr_merged = stats->merged[BLOCK_ACCT_WRITE];
+        ds->zone_append_merged = stats->merged[BLOCK_ACCT_ZONE_APPEND];
+        ds->unmap_merged = stats->merged[BLOCK_ACCT_UNMAP];
+        ds->flush_operations = stats->nr_ops[BLOCK_ACCT_FLUSH];
+        ds->wr_total_time_ns = stats->total_time_ns[BLOCK_ACCT_WRITE];
+        ds->zone_append_total_time_ns =
+            stats->total_time_ns[BLOCK_ACCT_ZONE_APPEND];
+        ds->rd_total_time_ns = stats->total_time_ns[BLOCK_ACCT_READ];
+        ds->flush_total_time_ns = stats->total_time_ns[BLOCK_ACCT_FLUSH];
+        ds->unmap_total_time_ns = stats->total_time_ns[BLOCK_ACCT_UNMAP];
+
+        ds->has_idle_time_ns = stats->last_access_time_ns > 0;
+        if (ds->has_idle_time_ns) {
+            ds->idle_time_ns = block_acct_idle_time_ns(stats);
+        }
 
-    ds->account_invalid = stats->account_invalid;
-    ds->account_failed = stats->account_failed;
+        ds->account_invalid = stats->account_invalid;
+        ds->account_failed = stats->account_failed;
+
+        hgram = stats->latency_histogram;
+        ds->rd_latency_histogram
+            = bdrv_latency_histogram_stats(&hgram[BLOCK_ACCT_READ]);
+        ds->wr_latency_histogram
+            = bdrv_latency_histogram_stats(&hgram[BLOCK_ACCT_WRITE]);
+        ds->zone_append_latency_histogram
+            = bdrv_latency_histogram_stats(&hgram[BLOCK_ACCT_ZONE_APPEND]);
+        ds->flush_latency_histogram
+            = bdrv_latency_histogram_stats(&hgram[BLOCK_ACCT_FLUSH]);
+    }
 
     while ((ts = block_acct_interval_next(stats, ts))) {
         BlockDeviceTimedStats *dev_stats = g_malloc0(sizeof(*dev_stats));
-- 
2.53.0

Re: [PATCH 2/3] block/qapi: take stats->lock when reading BlockAcctStats for query-blockstats

[Andrey Drobyshev]

On 7/23/26 3:37 PM, Denis V. Lunev wrote:
> bdrv_query_blk_stats() reads nr_bytes[], nr_ops[], failed_ops[],
> invalid_ops[], merged[], total_time_ns[], last_access_time_ns, and
> the latency histogram bins directly off BlockAcctStats, without
> taking stats->lock. block_account_one_io() updates the same fields
> under that lock from whatever iothread completes the I/O, so this is
> an unsynchronized read of concurrently-updated state.
> 
> This is a narrower issue than the one fixed for the histogram
> setters. query-blockstats and block-latency-histogram-set are both
> plain (non-allow-oob) QMP commands, so they always run serialized on
> the monitor thread and can never execute concurrently with each
> other.
> 
> Take stats->lock around the read for consistency.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Hanna Reitz <hreitz@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  block/qapi.c | 102 ++++++++++++++++++++++++++++++---------------------
>  1 file changed, 60 insertions(+), 42 deletions(-)
> 
> diff --git a/block/qapi.c b/block/qapi.c
> index eabfbfc258..c3f20832b7 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -535,49 +535,67 @@ static void bdrv_query_blk_stats(BlockDeviceStats *ds, BlockBackend *blk)
>      BlockAcctTimedStats *ts = NULL;
>      BlockLatencyHistogram *hgram;
>  
> -    ds->rd_bytes = stats->nr_bytes[BLOCK_ACCT_READ];
> -    ds->wr_bytes = stats->nr_bytes[BLOCK_ACCT_WRITE];
> -    ds->zone_append_bytes = stats->nr_bytes[BLOCK_ACCT_ZONE_APPEND];
> -    ds->unmap_bytes = stats->nr_bytes[BLOCK_ACCT_UNMAP];
> -    ds->rd_operations = stats->nr_ops[BLOCK_ACCT_READ];
> -    ds->wr_operations = stats->nr_ops[BLOCK_ACCT_WRITE];
> -    ds->zone_append_operations = stats->nr_ops[BLOCK_ACCT_ZONE_APPEND];
> -    ds->unmap_operations = stats->nr_ops[BLOCK_ACCT_UNMAP];
> -
> -    ds->failed_rd_operations = stats->failed_ops[BLOCK_ACCT_READ];
> -    ds->failed_wr_operations = stats->failed_ops[BLOCK_ACCT_WRITE];
> -    ds->failed_zone_append_operations =
> -        stats->failed_ops[BLOCK_ACCT_ZONE_APPEND];
> -    ds->failed_flush_operations = stats->failed_ops[BLOCK_ACCT_FLUSH];
> -    ds->failed_unmap_operations = stats->failed_ops[BLOCK_ACCT_UNMAP];
> -
> -    ds->invalid_rd_operations = stats->invalid_ops[BLOCK_ACCT_READ];
> -    ds->invalid_wr_operations = stats->invalid_ops[BLOCK_ACCT_WRITE];
> -    ds->invalid_zone_append_operations =
> -        stats->invalid_ops[BLOCK_ACCT_ZONE_APPEND];
> -    ds->invalid_flush_operations =
> -        stats->invalid_ops[BLOCK_ACCT_FLUSH];
> -    ds->invalid_unmap_operations = stats->invalid_ops[BLOCK_ACCT_UNMAP];
> -
> -    ds->rd_merged = stats->merged[BLOCK_ACCT_READ];
> -    ds->wr_merged = stats->merged[BLOCK_ACCT_WRITE];
> -    ds->zone_append_merged = stats->merged[BLOCK_ACCT_ZONE_APPEND];
> -    ds->unmap_merged = stats->merged[BLOCK_ACCT_UNMAP];
> -    ds->flush_operations = stats->nr_ops[BLOCK_ACCT_FLUSH];
> -    ds->wr_total_time_ns = stats->total_time_ns[BLOCK_ACCT_WRITE];
> -    ds->zone_append_total_time_ns =
> -        stats->total_time_ns[BLOCK_ACCT_ZONE_APPEND];
> -    ds->rd_total_time_ns = stats->total_time_ns[BLOCK_ACCT_READ];
> -    ds->flush_total_time_ns = stats->total_time_ns[BLOCK_ACCT_FLUSH];
> -    ds->unmap_total_time_ns = stats->total_time_ns[BLOCK_ACCT_UNMAP];
> -
> -    ds->has_idle_time_ns = stats->last_access_time_ns > 0;
> -    if (ds->has_idle_time_ns) {
> -        ds->idle_time_ns = block_acct_idle_time_ns(stats);
> -    }
> +    /*
> +     * nr_bytes[] etc. are mutated under stats->lock by block_account_one_io()
> +     * from an iothread, and latency_histogram[].{nbins,boundaries,bins} can
> +     * be freed and reallocated by a concurrent block_latency_histogram_set()
> +     * on the monitor thread, so every read here needs the same lock.
> +     */
> +    WITH_QEMU_LOCK_GUARD(&stats->lock) {
> +        ds->rd_bytes = stats->nr_bytes[BLOCK_ACCT_READ];
> +        ds->wr_bytes = stats->nr_bytes[BLOCK_ACCT_WRITE];
> +        ds->zone_append_bytes = stats->nr_bytes[BLOCK_ACCT_ZONE_APPEND];
> +        ds->unmap_bytes = stats->nr_bytes[BLOCK_ACCT_UNMAP];
> +        ds->rd_operations = stats->nr_ops[BLOCK_ACCT_READ];
> +        ds->wr_operations = stats->nr_ops[BLOCK_ACCT_WRITE];
> +        ds->zone_append_operations = stats->nr_ops[BLOCK_ACCT_ZONE_APPEND];
> +        ds->unmap_operations = stats->nr_ops[BLOCK_ACCT_UNMAP];
> +
> +        ds->failed_rd_operations = stats->failed_ops[BLOCK_ACCT_READ];
> +        ds->failed_wr_operations = stats->failed_ops[BLOCK_ACCT_WRITE];
> +        ds->failed_zone_append_operations =
> +            stats->failed_ops[BLOCK_ACCT_ZONE_APPEND];
> +        ds->failed_flush_operations = stats->failed_ops[BLOCK_ACCT_FLUSH];
> +        ds->failed_unmap_operations = stats->failed_ops[BLOCK_ACCT_UNMAP];
> +
> +        ds->invalid_rd_operations = stats->invalid_ops[BLOCK_ACCT_READ];
> +        ds->invalid_wr_operations = stats->invalid_ops[BLOCK_ACCT_WRITE];
> +        ds->invalid_zone_append_operations =
> +            stats->invalid_ops[BLOCK_ACCT_ZONE_APPEND];
> +        ds->invalid_flush_operations =
> +            stats->invalid_ops[BLOCK_ACCT_FLUSH];
> +        ds->invalid_unmap_operations = stats->invalid_ops[BLOCK_ACCT_UNMAP];
> +
> +        ds->rd_merged = stats->merged[BLOCK_ACCT_READ];
> +        ds->wr_merged = stats->merged[BLOCK_ACCT_WRITE];
> +        ds->zone_append_merged = stats->merged[BLOCK_ACCT_ZONE_APPEND];
> +        ds->unmap_merged = stats->merged[BLOCK_ACCT_UNMAP];
> +        ds->flush_operations = stats->nr_ops[BLOCK_ACCT_FLUSH];
> +        ds->wr_total_time_ns = stats->total_time_ns[BLOCK_ACCT_WRITE];
> +        ds->zone_append_total_time_ns =
> +            stats->total_time_ns[BLOCK_ACCT_ZONE_APPEND];
> +        ds->rd_total_time_ns = stats->total_time_ns[BLOCK_ACCT_READ];
> +        ds->flush_total_time_ns = stats->total_time_ns[BLOCK_ACCT_FLUSH];
> +        ds->unmap_total_time_ns = stats->total_time_ns[BLOCK_ACCT_UNMAP];
> +
> +        ds->has_idle_time_ns = stats->last_access_time_ns > 0;
> +        if (ds->has_idle_time_ns) {
> +            ds->idle_time_ns = block_acct_idle_time_ns(stats);
> +        }
>  
> -    ds->account_invalid = stats->account_invalid;
> -    ds->account_failed = stats->account_failed;
> +        ds->account_invalid = stats->account_invalid;
> +        ds->account_failed = stats->account_failed;
> +
> +        hgram = stats->latency_histogram;
> +        ds->rd_latency_histogram
> +            = bdrv_latency_histogram_stats(&hgram[BLOCK_ACCT_READ]);
> +        ds->wr_latency_histogram
> +            = bdrv_latency_histogram_stats(&hgram[BLOCK_ACCT_WRITE]);
> +        ds->zone_append_latency_histogram
> +            = bdrv_latency_histogram_stats(&hgram[BLOCK_ACCT_ZONE_APPEND]);
> +        ds->flush_latency_histogram
> +            = bdrv_latency_histogram_stats(&hgram[BLOCK_ACCT_FLUSH]);
> +    }

Smth is off with this hunk.  These calls to
bdrv_latency_histogram_stats() pre-exist below (after the while loop).
If they need to be protected by the lock as well, we should remove those
duplicating calls below.

>  
>      while ((ts = block_acct_interval_next(stats, ts))) {
>          BlockDeviceTimedStats *dev_stats = g_malloc0(sizeof(*dev_stats));

Also in the while loop we call timed_average_{min,max,avg} with no
locks, whereas block_account_one_io() does update the intervals under
the lock.  Shouldn't we move the while loop itself under the lock guard?

Andrey

[PATCH 3/3] tests/unit: add reproducer for BlockAcctStats histogram locking race

[Denis V. Lunev]

block_latency_histogram_set() and block_latency_histograms_clear()
replace BlockLatencyHistogram's nbins/boundaries/bins without taking
stats->lock, while block_account_one_io() reads those same fields
under that lock from whatever iothread completes the I/O.

Add a test that races two real threads against
block_latency_histogram_set() and
block_acct_start()/block_acct_done() on the same BlockAcctStats.
Applied here it passes, since the previous two commits already take
the lock; reverting them locally reproduces the abort this series
fixes, in about a second.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Hanna Reitz <hreitz@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 tests/unit/meson.build             |   1 +
 tests/unit/test-block-accounting.c | 114 +++++++++++++++++++++++++++++
 2 files changed, 115 insertions(+)
 create mode 100644 tests/unit/test-block-accounting.c

diff --git a/tests/unit/meson.build b/tests/unit/meson.build
index 5ba6b1a230..dc3fb954c0 100644
--- a/tests/unit/meson.build
+++ b/tests/unit/meson.build
@@ -75,6 +75,7 @@ if have_block
     'test-blockjob': [testblock],
     'test-blockjob-txn': [testblock],
     'test-block-backend': [testblock],
+    'test-block-accounting': [testblock],
     'test-block-iothread': [testblock],
     'test-write-threshold': [testblock],
     'test-crypto-hash': [crypto],
diff --git a/tests/unit/test-block-accounting.c b/tests/unit/test-block-accounting.c
new file mode 100644
index 0000000000..e749b73749
--- /dev/null
+++ b/tests/unit/test-block-accounting.c
@@ -0,0 +1,114 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * BlockAcctStats latency histogram locking regression test
+ *
+ * Copyright (c) 2026 Virtuozzo International GmbH.
+ *
+ * Regression test for missing stats->lock in
+ * block_latency_histogram_set()/block_latency_histograms_clear(),
+ * racing block_account_one_io() reading the same fields from an
+ * iothread. Aborts reliably before the fix, passes after it.
+ */
+
+#include "qemu/osdep.h"
+#include "block/block.h"
+#include "block/accounting.h"
+#include "system/block-backend.h"
+#include "system/block-backend-io.h"
+#include "qapi/error.h"
+#include "qemu/main-loop.h"
+#include "qemu/thread.h"
+
+#define RACE_DURATION_MS 2000
+#define NUM_READER_THREADS 8
+
+static bool stop_workers;
+
+/*
+ * Different bin counts, so the writer's g_free()/g_new() churn can be
+ * caught mid-update. Values are small enough (nanoseconds) that plain
+ * back-to-back start/done calls exercise every bin without sleeping.
+ */
+static uint64List boundaries_a[] = {
+    { .next = &boundaries_a[1], .value = 1000 },
+    { .next = &boundaries_a[2], .value = 5000 },
+    { .next = NULL,             .value = 50000 },
+};
+
+static uint64List boundaries_b[] = {
+    { .next = &boundaries_b[1], .value = 800 },
+    { .next = &boundaries_b[2], .value = 3000 },
+    { .next = &boundaries_b[3], .value = 20000 },
+    { .next = NULL,             .value = 200000 },
+};
+
+static void *writer_thread(void *opaque)
+{
+    BlockAcctStats *stats = opaque;
+
+    while (!qatomic_read(&stop_workers)) {
+        block_latency_histogram_set(stats, BLOCK_ACCT_READ, boundaries_a);
+        block_latency_histogram_set(stats, BLOCK_ACCT_READ, boundaries_b);
+    }
+
+    return NULL;
+}
+
+static void *reader_thread(void *opaque)
+{
+    BlockAcctStats *stats = opaque;
+
+    while (!qatomic_read(&stop_workers)) {
+        BlockAcctCookie cookie;
+
+        block_acct_start(stats, &cookie, 4096, BLOCK_ACCT_READ);
+        block_acct_done(stats, &cookie);
+    }
+
+    return NULL;
+}
+
+static void test_latency_histogram_race(void)
+{
+    BlockBackend *blk = blk_new(qemu_get_aio_context(),
+                                BLK_PERM_ALL, BLK_PERM_ALL);
+    BlockAcctStats *stats = blk_get_stats(blk);
+    QemuThread writer, readers[NUM_READER_THREADS];
+    int i;
+
+    /* Histogram has to be enabled (bins != NULL) before racing it. */
+    g_assert(block_latency_histogram_set(stats, BLOCK_ACCT_READ,
+                                         boundaries_a) == 0);
+
+    stop_workers = false;
+    qemu_thread_create(&writer, "hist-writer", writer_thread, stats,
+                       QEMU_THREAD_JOINABLE);
+    for (i = 0; i < NUM_READER_THREADS; i++) {
+        qemu_thread_create(&readers[i], "hist-reader", reader_thread, stats,
+                           QEMU_THREAD_JOINABLE);
+    }
+
+    g_usleep(RACE_DURATION_MS * 1000);
+    qatomic_set(&stop_workers, true);
+
+    qemu_thread_join(&writer);
+    for (i = 0; i < NUM_READER_THREADS; i++) {
+        qemu_thread_join(&readers[i]);
+    }
+
+    blk_unref(blk);
+}
+
+int main(int argc, char **argv)
+{
+    bdrv_init();
+    qemu_init_main_loop(&error_abort);
+
+    g_test_init(&argc, &argv, NULL);
+
+    g_test_add_func("/block-accounting/latency_histogram_race",
+                    test_latency_histogram_race);
+
+    return g_test_run();
+}
-- 
2.53.0

Re: [PATCH 3/3] tests/unit: add reproducer for BlockAcctStats histogram locking race

[Andrey Drobyshev]

On 7/23/26 3:37 PM, Denis V. Lunev wrote:
> block_latency_histogram_set() and block_latency_histograms_clear()
> replace BlockLatencyHistogram's nbins/boundaries/bins without taking
> stats->lock, while block_account_one_io() reads those same fields
> under that lock from whatever iothread completes the I/O.
> 
> Add a test that races two real threads against
> block_latency_histogram_set() and
> block_acct_start()/block_acct_done() on the same BlockAcctStats.
> Applied here it passes, since the previous two commits already take
> the lock; reverting them locally reproduces the abort this series
> fixes, in about a second.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Hanna Reitz <hreitz@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  tests/unit/meson.build             |   1 +
>  tests/unit/test-block-accounting.c | 114 +++++++++++++++++++++++++++++
>  2 files changed, 115 insertions(+)
>  create mode 100644 tests/unit/test-block-accounting.c
> 
> diff --git a/tests/unit/meson.build b/tests/unit/meson.build
> index 5ba6b1a230..dc3fb954c0 100644
> --- a/tests/unit/meson.build
> +++ b/tests/unit/meson.build
> @@ -75,6 +75,7 @@ if have_block
>      'test-blockjob': [testblock],
>      'test-blockjob-txn': [testblock],
>      'test-block-backend': [testblock],
> +    'test-block-accounting': [testblock],
>      'test-block-iothread': [testblock],
>      'test-write-threshold': [testblock],
>      'test-crypto-hash': [crypto],
> diff --git a/tests/unit/test-block-accounting.c b/tests/unit/test-block-accounting.c
> new file mode 100644
> index 0000000000..e749b73749
> --- /dev/null
> +++ b/tests/unit/test-block-accounting.c
> @@ -0,0 +1,114 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * BlockAcctStats latency histogram locking regression test
> + *
> + * Copyright (c) 2026 Virtuozzo International GmbH.
> + *
> + * Regression test for missing stats->lock in
> + * block_latency_histogram_set()/block_latency_histograms_clear(),
> + * racing block_account_one_io() reading the same fields from an
> + * iothread. Aborts reliably before the fix, passes after it.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "block/block.h"
> +#include "block/accounting.h"
> +#include "system/block-backend.h"
> +#include "system/block-backend-io.h"
> +#include "qapi/error.h"
> +#include "qemu/main-loop.h"
> +#include "qemu/thread.h"
> +
> +#define RACE_DURATION_MS 2000
> +#define NUM_READER_THREADS 8
> +
> +static bool stop_workers;
> +
> +/*
> + * Different bin counts, so the writer's g_free()/g_new() churn can be
> + * caught mid-update. Values are small enough (nanoseconds) that plain
> + * back-to-back start/done calls exercise every bin without sleeping.
> + */
> +static uint64List boundaries_a[] = {
> +    { .next = &boundaries_a[1], .value = 1000 },
> +    { .next = &boundaries_a[2], .value = 5000 },
> +    { .next = NULL,             .value = 50000 },
> +};
> +
> +static uint64List boundaries_b[] = {
> +    { .next = &boundaries_b[1], .value = 800 },
> +    { .next = &boundaries_b[2], .value = 3000 },
> +    { .next = &boundaries_b[3], .value = 20000 },
> +    { .next = NULL,             .value = 200000 },
> +};
> +
> +static void *writer_thread(void *opaque)
> +{
> +    BlockAcctStats *stats = opaque;
> +
> +    while (!qatomic_read(&stop_workers)) {
> +        block_latency_histogram_set(stats, BLOCK_ACCT_READ, boundaries_a);
> +        block_latency_histogram_set(stats, BLOCK_ACCT_READ, boundaries_b);

We fixed block_latency_histograms_clear() in the 1st patch but don't
cover it here in the test.  Should we add a call to
block_latency_histograms_clear(stats) here in the end of while loop to
get it covered?

Andrey

> +    }
> +
> +    return NULL;
> +}
> +
> +static void *reader_thread(void *opaque)
> +{
> +    BlockAcctStats *stats = opaque;
> +
> +    while (!qatomic_read(&stop_workers)) {
> +        BlockAcctCookie cookie;
> +
> +        block_acct_start(stats, &cookie, 4096, BLOCK_ACCT_READ);
> +        block_acct_done(stats, &cookie);
> +    }
> +
> +    return NULL;
> +}
> +
> +static void test_latency_histogram_race(void)
> +{
> +    BlockBackend *blk = blk_new(qemu_get_aio_context(),
> +                                BLK_PERM_ALL, BLK_PERM_ALL);
> +    BlockAcctStats *stats = blk_get_stats(blk);
> +    QemuThread writer, readers[NUM_READER_THREADS];
> +    int i;
> +
> +    /* Histogram has to be enabled (bins != NULL) before racing it. */
> +    g_assert(block_latency_histogram_set(stats, BLOCK_ACCT_READ,
> +                                         boundaries_a) == 0);
> +
> +    stop_workers = false;
> +    qemu_thread_create(&writer, "hist-writer", writer_thread, stats,
> +                       QEMU_THREAD_JOINABLE);
> +    for (i = 0; i < NUM_READER_THREADS; i++) {
> +        qemu_thread_create(&readers[i], "hist-reader", reader_thread, stats,
> +                           QEMU_THREAD_JOINABLE);
> +    }
> +
> +    g_usleep(RACE_DURATION_MS * 1000);
> +    qatomic_set(&stop_workers, true);
> +
> +    qemu_thread_join(&writer);
> +    for (i = 0; i < NUM_READER_THREADS; i++) {
> +        qemu_thread_join(&readers[i]);
> +    }
> +
> +    blk_unref(blk);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    bdrv_init();
> +    qemu_init_main_loop(&error_abort);
> +
> +    g_test_init(&argc, &argv, NULL);
> +
> +    g_test_add_func("/block-accounting/latency_histogram_race",
> +                    test_latency_histogram_race);
> +
> +    return g_test_run();
> +}