From: Denis V. Lunev <[email protected]> throttle_group_register_tgm() stores tgm->throttle_state before it takes tg->lock, so the I/O path can see a member whose group still has NULL tokens[] and an empty member list. A request in an iothread then reaches throttle_group_co_io_limits_intercept(), wins tg->lock ahead of the registering thread, and next_throttle_token() passes the NULL token to throttle_group_next_tgm(), which dereferences it:
throttle_group_next_tgm (tgm=0x0) at block/throttle-groups.c:185 next_throttle_token (tgm=..., direction=THROTTLE_READ) throttle_group_co_io_limits_intercept (tgm=..., bytes=8192, ...) blk_co_do_preadv_part (blk=..., offset=..., bytes=8192, ...) blk_aio_read_entry (opaque=...) coroutine_trampoline (i0=..., i1=...) blk_io_limits_enable() does not drain the BlockBackend, unlike its disable counterpart, so nothing keeps requests away while the group is built. A guest probing a disk that libvirt has just attached, at the moment the QoS settings for it are applied, is enough to hit this. Publish tgm->throttle_state with a release store once the member is linked into the group and its timers exist, and read it with an acquire load on the two unlocked I/O paths. A request that sees the new pointer then also sees a fully built group. Cc: Kevin Wolf <[email protected]> Cc: Hanna Reitz <[email protected]> Cc: Alberto Garcia <[email protected]> Signed-off-by: Denis V. Lunev <[email protected]> --- block/block-backend.c | 12 ++++++------ block/throttle-groups.c | 4 +++- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/block/block-backend.c b/block/block-backend.c index 37ba7e9fc4..5e59bac1a3 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -1338,6 +1338,7 @@ blk_co_do_preadv_part(BlockBackend *blk, int64_t offset, int64_t bytes, QEMUIOVector *qiov, size_t qiov_offset, BdrvRequestFlags flags) { + ThrottleGroupMember *tgm = &blk->public.throttle_group_member; int ret; BlockDriverState *bs; IO_CODE(); @@ -1357,9 +1358,8 @@ blk_co_do_preadv_part(BlockBackend *blk, int64_t offset, int64_t bytes, bdrv_inc_in_flight(bs); /* throttling disk I/O */ - if (blk->public.throttle_group_member.throttle_state) { - throttle_group_co_io_limits_intercept(&blk->public.throttle_group_member, - bytes, THROTTLE_READ); + if (qatomic_load_acquire(&tgm->throttle_state)) { + throttle_group_co_io_limits_intercept(tgm, bytes, THROTTLE_READ); } ret = bdrv_co_preadv_part(blk->root, offset, bytes, qiov, qiov_offset, @@ -1413,6 +1413,7 @@ blk_co_do_pwritev_part(BlockBackend *blk, int64_t offset, int64_t bytes, QEMUIOVector *qiov, size_t qiov_offset, BdrvRequestFlags flags) { + ThrottleGroupMember *tgm = &blk->public.throttle_group_member; int ret; BlockDriverState *bs; IO_CODE(); @@ -1431,9 +1432,8 @@ blk_co_do_pwritev_part(BlockBackend *blk, int64_t offset, int64_t bytes, bdrv_inc_in_flight(bs); /* throttling disk I/O */ - if (blk->public.throttle_group_member.throttle_state) { - throttle_group_co_io_limits_intercept(&blk->public.throttle_group_member, - bytes, THROTTLE_WRITE); + if (qatomic_load_acquire(&tgm->throttle_state)) { + throttle_group_co_io_limits_intercept(tgm, bytes, THROTTLE_WRITE); } if (!blk->enable_write_cache) { diff --git a/block/throttle-groups.c b/block/throttle-groups.c index 805e47270c..faf74a969d 100644 --- a/block/throttle-groups.c +++ b/block/throttle-groups.c @@ -581,7 +581,6 @@ void throttle_group_register_tgm(ThrottleGroupMember *tgm, ThrottleState *ts = throttle_group_incref(groupname); ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts); - tgm->throttle_state = ts; tgm->aio_context = ctx; qatomic_set(&tgm->restart_pending, 0); @@ -602,6 +601,9 @@ void throttle_group_register_tgm(ThrottleGroupMember *tgm, read_timer_cb, write_timer_cb, tgm); + + /* The I/O path reads this without tg->lock, so publish it last */ + qatomic_store_release(&tgm->throttle_state, ts); } /* Unregister a ThrottleGroupMember from its group, removing it from the list, -- 2.53.0
