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

[PATCH v2 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.

v1 -> v2
* patch 1, 2: WITH_QEMU_LOCK_GUARD -> plain lock()/unlock(), pure
  additions, nothing reindented or moved
* patch 3: writer thread now also calls
  block_latency_histograms_clear(), the other function patch 1 fixes
* patch 2: take stats->lock for bdrv_query_blk_stats()'s whole call
  instead of per-section, since the interval loop's
  timed_average_min/max/avg() calls race timed_average_account() the
  same way the already-locked fields did; block_acct_queue_depth()
  now requires the caller to hold the lock instead of taking it
  itself, since bdrv_query_blk_stats() is its only caller

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>
CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

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                 |  11 ++-
 block/qapi.c                       |   3 +
 include/block/accounting.h         |   1 +
 tests/unit/meson.build             |   1 +
 tests/unit/test-block-accounting.c | 115 +++++++++++++++++++++++++++++
 5 files changed, 129 insertions(+), 2 deletions(-)
 create mode 100644 tests/unit/test-block-accounting.c

-- 
2.53.0

[PATCH v2 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>
CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 block/accounting.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/block/accounting.c b/block/accounting.c
index f00fe99740..6e06c7609e 100644
--- a/block/accounting.c
+++ b/block/accounting.c
@@ -194,6 +194,8 @@ int block_latency_histogram_set(BlockAcctStats *stats, enum BlockAcctType type,
         return -EINVAL;
     }
 
+    qemu_mutex_lock(&stats->lock);
+
     hist->nbins = new_nbins;
     g_free(hist->boundaries);
     hist->boundaries = g_new(uint64_t, hist->nbins - 1);
@@ -206,6 +208,8 @@ int block_latency_histogram_set(BlockAcctStats *stats, enum BlockAcctType type,
     g_free(hist->bins);
     hist->bins = g_new0(uint64_t, hist->nbins);
 
+    qemu_mutex_unlock(&stats->lock);
+
     return 0;
 }
 
@@ -213,12 +217,16 @@ void block_latency_histograms_clear(BlockAcctStats *stats)
 {
     int i;
 
+    qemu_mutex_lock(&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));
     }
+
+    qemu_mutex_unlock(&stats->lock);
 }
 
 static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie,
-- 
2.53.0

[PATCH v2 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>
CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 tests/unit/meson.build             |   1 +
 tests/unit/test-block-accounting.c | 115 +++++++++++++++++++++++++++++
 2 files changed, 116 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..7aae491cfc
--- /dev/null
+++ b/tests/unit/test-block-accounting.c
@@ -0,0 +1,115 @@
+/*
+ * 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);
+        block_latency_histograms_clear(stats);
+    }
+
+    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 v2 0/3] block/accounting: fix unlocked latency histogram reconfiguration races

[Kevin Wolf]

Am 24.07.2026 um 13:13 hat Denis V. Lunev geschrieben:
> 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.
> 
> v1 -> v2
> * patch 1, 2: WITH_QEMU_LOCK_GUARD -> plain lock()/unlock(), pure
>   additions, nothing reindented or moved
> * patch 3: writer thread now also calls
>   block_latency_histograms_clear(), the other function patch 1 fixes
> * patch 2: take stats->lock for bdrv_query_blk_stats()'s whole call
>   instead of per-section, since the interval loop's
>   timed_average_min/max/avg() calls race timed_average_account() the
>   same way the already-locked fields did; block_acct_queue_depth()
>   now requires the caller to hold the lock instead of taking it
>   itself, since bdrv_query_blk_stats() is its only caller

Thanks, applied to the block branch.

Kevin