From 32125b14606a454ed109ea6d9da32c747e94926f Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 3 Feb 2023 16:21:41 +0100 Subject: mirror: Fix access of uninitialised fields during start bdrv_mirror_top_pwritev() accesses the job object when active mirroring is enabled. It disables this code during early initialisation while s->job isn't set yet. However, s->job is still set way too early when the job object isn't fully initialised. For example, &s->ops_in_flight isn't initialised yet and the in_flight bitmap doesn't exist yet. This causes crashes when a write request comes in too early. Move the assignment of s->job to when the mirror job is actually fully initialised to make sure that the mirror_top driver doesn't access it too early. Signed-off-by: Kevin Wolf Message-Id: <20230203152202.49054-3-kwolf@redhat.com> Reviewed-by: Emanuele Giuseppe Esposito Reviewed-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Kevin Wolf --- block/mirror.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'block/mirror.c') diff --git a/block/mirror.c b/block/mirror.c index ab326b6..fbbb4f6 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -896,6 +896,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp) { MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job); BlockDriverState *bs = s->mirror_top_bs->backing->bs; + MirrorBDSOpaque *mirror_top_opaque = s->mirror_top_bs->opaque; BlockDriverState *target_bs = blk_bs(s->target); bool need_drain = true; BlockDeviceIoStatus iostatus; @@ -985,6 +986,12 @@ static int coroutine_fn mirror_run(Job *job, Error **errp) } } + /* + * Only now the job is fully initialised and mirror_top_bs should start + * accessing it. + */ + mirror_top_opaque->job = s; + assert(!s->dbi); s->dbi = bdrv_dirty_iter_new(s->dirty_bitmap); for (;;) { @@ -1704,7 +1711,6 @@ static BlockJob *mirror_start_job( if (!s) { goto fail; } - bs_opaque->job = s; /* The block job now has a reference to this node */ bdrv_unref(mirror_top_bs); -- cgit v1.1 From 7ff9579e60a42e253c6bbbd7ba613f19cc007259 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 3 Feb 2023 16:21:43 +0100 Subject: block: Mark bdrv_co_block_status() and callers GRAPH_RDLOCK This adds GRAPH_RDLOCK annotations to declare that callers of bdrv_co_block_status() need to hold a reader lock for the graph. For some places, we know that they will hold the lock, but we don't have the GRAPH_RDLOCK annotations yet. In this case, add assume_graph_lock() with a FIXME comment. These places will be removed once everything is properly annotated. Signed-off-by: Emanuele Giuseppe Esposito Signed-off-by: Kevin Wolf Message-Id: <20230203152202.49054-5-kwolf@redhat.com> Reviewed-by: Emanuele Giuseppe Esposito Signed-off-by: Kevin Wolf --- block/mirror.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) (limited to 'block/mirror.c') diff --git a/block/mirror.c b/block/mirror.c index fbbb4f6..94c523a 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -558,9 +558,11 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s) MirrorMethod mirror_method = MIRROR_METHOD_COPY; assert(!(offset % s->granularity)); - ret = bdrv_block_status_above(source, NULL, offset, - nb_chunks * s->granularity, - &io_bytes, NULL, NULL); + WITH_GRAPH_RDLOCK_GUARD() { + ret = bdrv_block_status_above(source, NULL, offset, + nb_chunks * s->granularity, + &io_bytes, NULL, NULL); + } if (ret < 0) { io_bytes = MIN(nb_chunks * s->granularity, max_io_bytes); } else if (ret & BDRV_BLOCK_DATA) { @@ -863,8 +865,10 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s) return 0; } - ret = bdrv_is_allocated_above(bs, s->base_overlay, true, offset, bytes, - &count); + WITH_GRAPH_RDLOCK_GUARD() { + ret = bdrv_is_allocated_above(bs, s->base_overlay, true, offset, + bytes, &count); + } if (ret < 0) { return ret; } -- cgit v1.1 From 880953493386a69416d2e1cdc063c670585a03ac Mon Sep 17 00:00:00 2001 From: Emanuele Giuseppe Esposito Date: Fri, 3 Feb 2023 16:21:46 +0100 Subject: block: Mark bdrv_co_flush() and callers GRAPH_RDLOCK This adds GRAPH_RDLOCK annotations to declare that callers of bdrv_co_flush() need to hold a reader lock for the graph. For some places, we know that they will hold the lock, but we don't have the GRAPH_RDLOCK annotations yet. In this case, add assume_graph_lock() with a FIXME comment. These places will be removed once everything is properly annotated. Signed-off-by: Emanuele Giuseppe Esposito Signed-off-by: Kevin Wolf Message-Id: <20230203152202.49054-8-kwolf@redhat.com> Reviewed-by: Emanuele Giuseppe Esposito Signed-off-by: Kevin Wolf --- block/mirror.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'block/mirror.c') diff --git a/block/mirror.c b/block/mirror.c index 94c523a..d1d79f2 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -1535,7 +1535,7 @@ static int coroutine_fn bdrv_mirror_top_pwritev(BlockDriverState *bs, return ret; } -static int coroutine_fn bdrv_mirror_top_flush(BlockDriverState *bs) +static int coroutine_fn GRAPH_RDLOCK bdrv_mirror_top_flush(BlockDriverState *bs) { if (bs->backing == NULL) { /* we can be here after failed bdrv_append in mirror_start_job */ -- cgit v1.1 From 9a5a1c621ed72161abcf461d46c7b7b7f97938bf Mon Sep 17 00:00:00 2001 From: Emanuele Giuseppe Esposito Date: Fri, 3 Feb 2023 16:21:47 +0100 Subject: block: Mark bdrv_co_pdiscard() and callers GRAPH_RDLOCK This adds GRAPH_RDLOCK annotations to declare that callers of bdrv_co_pdiscard() need to hold a reader lock for the graph. For some places, we know that they will hold the lock, but we don't have the GRAPH_RDLOCK annotations yet. In this case, add assume_graph_lock() with a FIXME comment. These places will be removed once everything is properly annotated. Signed-off-by: Kevin Wolf Message-Id: <20230203152202.49054-9-kwolf@redhat.com> Reviewed-by: Emanuele Giuseppe Esposito Signed-off-by: Kevin Wolf --- block/mirror.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) (limited to 'block/mirror.c') diff --git a/block/mirror.c b/block/mirror.c index d1d79f2..b67e8b1 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -1443,9 +1443,10 @@ static int coroutine_fn bdrv_mirror_top_preadv(BlockDriverState *bs, return bdrv_co_preadv(bs->backing, offset, bytes, qiov, flags); } -static int coroutine_fn bdrv_mirror_top_do_write(BlockDriverState *bs, - MirrorMethod method, uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, - int flags) +static int coroutine_fn GRAPH_RDLOCK +bdrv_mirror_top_do_write(BlockDriverState *bs, MirrorMethod method, + uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, + int flags) { MirrorOp *op = NULL; MirrorBDSOpaque *s = bs->opaque; @@ -1503,6 +1504,8 @@ static int coroutine_fn bdrv_mirror_top_pwritev(BlockDriverState *bs, int ret = 0; bool copy_to_target = false; + assume_graph_lock(); /* FIXME */ + if (s->job) { copy_to_target = s->job->ret >= 0 && !job_is_cancelled(&s->job->common.job) && @@ -1547,12 +1550,13 @@ static int coroutine_fn GRAPH_RDLOCK bdrv_mirror_top_flush(BlockDriverState *bs) static int coroutine_fn bdrv_mirror_top_pwrite_zeroes(BlockDriverState *bs, int64_t offset, int64_t bytes, BdrvRequestFlags flags) { + assume_graph_lock(); /* FIXME */ return bdrv_mirror_top_do_write(bs, MIRROR_METHOD_ZERO, offset, bytes, NULL, flags); } -static int coroutine_fn bdrv_mirror_top_pdiscard(BlockDriverState *bs, - int64_t offset, int64_t bytes) +static int coroutine_fn GRAPH_RDLOCK +bdrv_mirror_top_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes) { return bdrv_mirror_top_do_write(bs, MIRROR_METHOD_DISCARD, offset, bytes, NULL, 0); -- cgit v1.1 From abaf8b750baef0337efb06c1d3465512b5d9b5dc Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 3 Feb 2023 16:21:48 +0100 Subject: block: Mark bdrv_co_pwrite_zeroes() and callers GRAPH_RDLOCK This adds GRAPH_RDLOCK annotations to declare that callers of bdrv_co_pwrite_zeroes() need to hold a reader lock for the graph. For some places, we know that they will hold the lock, but we don't have the GRAPH_RDLOCK annotations yet. In this case, add assume_graph_lock() with a FIXME comment. These places will be removed once everything is properly annotated. Signed-off-by: Kevin Wolf Message-Id: <20230203152202.49054-10-kwolf@redhat.com> Reviewed-by: Emanuele Giuseppe Esposito Signed-off-by: Kevin Wolf --- block/mirror.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'block/mirror.c') diff --git a/block/mirror.c b/block/mirror.c index b67e8b1..a6f4ec6 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -1547,10 +1547,10 @@ static int coroutine_fn GRAPH_RDLOCK bdrv_mirror_top_flush(BlockDriverState *bs) return bdrv_co_flush(bs->backing->bs); } -static int coroutine_fn bdrv_mirror_top_pwrite_zeroes(BlockDriverState *bs, - int64_t offset, int64_t bytes, BdrvRequestFlags flags) +static int coroutine_fn GRAPH_RDLOCK +bdrv_mirror_top_pwrite_zeroes(BlockDriverState *bs, int64_t offset, + int64_t bytes, BdrvRequestFlags flags) { - assume_graph_lock(); /* FIXME */ return bdrv_mirror_top_do_write(bs, MIRROR_METHOD_ZERO, offset, bytes, NULL, flags); } -- cgit v1.1 From b9b10c35e5c8bdb800601b142c44a4bd2da5a6d2 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 3 Feb 2023 16:21:50 +0100 Subject: block: Mark public read/write functions GRAPH_RDLOCK This adds GRAPH_RDLOCK annotations to declare that callers of bdrv_co_pread*/pwrite*() need to hold a reader lock for the graph. For some places, we know that they will hold the lock, but we don't have the GRAPH_RDLOCK annotations yet. In this case, add assume_graph_lock() with a FIXME comment. These places will be removed once everything is properly annotated. Signed-off-by: Kevin Wolf Message-Id: <20230203152202.49054-12-kwolf@redhat.com> Reviewed-by: Emanuele Giuseppe Esposito Signed-off-by: Kevin Wolf --- block/mirror.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) (limited to 'block/mirror.c') diff --git a/block/mirror.c b/block/mirror.c index a6f4ec6..ec5cd22 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -390,8 +390,10 @@ static void coroutine_fn mirror_co_read(void *opaque) op->is_in_flight = true; trace_mirror_one_iteration(s, op->offset, op->bytes); - ret = bdrv_co_preadv(s->mirror_top_bs->backing, op->offset, op->bytes, - &op->qiov, 0); + WITH_GRAPH_RDLOCK_GUARD() { + ret = bdrv_co_preadv(s->mirror_top_bs->backing, op->offset, op->bytes, + &op->qiov, 0); + } mirror_read_complete(op, ret); } @@ -1437,8 +1439,9 @@ static void coroutine_fn active_write_settle(MirrorOp *op) g_free(op); } -static int coroutine_fn bdrv_mirror_top_preadv(BlockDriverState *bs, - int64_t offset, int64_t bytes, QEMUIOVector *qiov, BdrvRequestFlags flags) +static int coroutine_fn GRAPH_RDLOCK +bdrv_mirror_top_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes, + QEMUIOVector *qiov, BdrvRequestFlags flags) { return bdrv_co_preadv(bs->backing, offset, bytes, qiov, flags); } @@ -1495,8 +1498,9 @@ out: return ret; } -static int coroutine_fn bdrv_mirror_top_pwritev(BlockDriverState *bs, - int64_t offset, int64_t bytes, QEMUIOVector *qiov, BdrvRequestFlags flags) +static int coroutine_fn GRAPH_RDLOCK +bdrv_mirror_top_pwritev(BlockDriverState *bs, int64_t offset, int64_t bytes, + QEMUIOVector *qiov, BdrvRequestFlags flags) { MirrorBDSOpaque *s = bs->opaque; QEMUIOVector bounce_qiov; @@ -1504,8 +1508,6 @@ static int coroutine_fn bdrv_mirror_top_pwritev(BlockDriverState *bs, int ret = 0; bool copy_to_target = false; - assume_graph_lock(); /* FIXME */ - if (s->job) { copy_to_target = s->job->ret >= 0 && !job_is_cancelled(&s->job->common.job) && -- cgit v1.1 From 8ab8140a04cf771d63e9754d6ba6c1e676bfe507 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 3 Feb 2023 16:22:02 +0100 Subject: block: Mark bdrv_co_refresh_total_sectors() and callers GRAPH_RDLOCK This adds GRAPH_RDLOCK annotations to declare that callers of bdrv_co_refresh_total_sectors() need to hold a reader lock for the graph. Signed-off-by: Kevin Wolf Message-Id: <20230203152202.49054-24-kwolf@redhat.com> Reviewed-by: Emanuele Giuseppe Esposito Signed-off-by: Kevin Wolf --- block/mirror.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'block/mirror.c') diff --git a/block/mirror.c b/block/mirror.c index ec5cd22..97c6a57 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -917,7 +917,10 @@ static int coroutine_fn mirror_run(Job *job, Error **errp) goto immediate_exit; } + bdrv_graph_co_rdlock(); s->bdev_length = bdrv_co_getlength(bs); + bdrv_graph_co_rdunlock(); + if (s->bdev_length < 0) { ret = s->bdev_length; goto immediate_exit; -- cgit v1.1