block/mirror.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)
We found an issue when doing block-commit with iothreads, which tries to
dereference a NULL pointer.
<main thread> |<IO thread>
|
mirror_start_job |
1. bdrv_ref(mirror_top_bs); |
bdrv_drained_begin(bs); |
bdrv_append(mirror_top_bs, |
bs, &local_err); |
bdrv_drained_end(bs); |
|
2. s = block_job_create(...); |
|bdrv_mirror_top_pwritev
| MirrorBDSOpaque *s = bs->opaque;
| bool copy_to_target;
| copy_to_target = s->job->ret >= 0 &&
| s->job->copy_mode ==
| MIRROR_COPY_MODE_WRITE_BLOCKING;
| (s->job is not NULL until 3!)
3. bs_opaque->job = s; |
Just moving step 2 & 3 before 1 can avoid this.
Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Peng Liang <liangpeng10@huawei.com>
---
block/mirror.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index e8e8844afc40..7c872be71149 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1600,6 +1600,16 @@ static BlockJob *mirror_start_job(
mirror_top_bs->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED |
BDRV_REQ_NO_FALLBACK;
bs_opaque = g_new0(MirrorBDSOpaque, 1);
+ /* Make sure that the source is not resized while the job is running */
+ s = block_job_create(job_id, driver, NULL, bs,
+ BLK_PERM_CONSISTENT_READ,
+ BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
+ BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD, speed,
+ creation_flags, cb, opaque, errp);
+ if (!s) {
+ goto fail;
+ }
+ bs_opaque->job = s;
mirror_top_bs->opaque = bs_opaque;
/* bdrv_append takes ownership of the mirror_top_bs reference, need to keep
@@ -1612,19 +1622,8 @@ static BlockJob *mirror_start_job(
if (local_err) {
bdrv_unref(mirror_top_bs);
error_propagate(errp, local_err);
- return NULL;
- }
-
- /* Make sure that the source is not resized while the job is running */
- s = block_job_create(job_id, driver, NULL, mirror_top_bs,
- BLK_PERM_CONSISTENT_READ,
- BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
- BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD, speed,
- creation_flags, cb, opaque, errp);
- if (!s) {
goto fail;
}
- bs_opaque->job = s;
/* The block job now has a reference to this node */
bdrv_unref(mirror_top_bs);
--
2.18.4
On 8/26/20 9:19 AM, Peng Liang wrote:
> We found an issue when doing block-commit with iothreads, which tries to
> dereference a NULL pointer.
>
I'm clearing out my patch backlog. I am a bit out of the loop on block
issues at the moment, did this issue get addressed in a later patch that
I am not seeing, or does it still need attention?
--js
> <main thread> |<IO thread>
> |
> mirror_start_job |
> 1. bdrv_ref(mirror_top_bs); |
> bdrv_drained_begin(bs); |
> bdrv_append(mirror_top_bs, |
> bs, &local_err); |
> bdrv_drained_end(bs); |
> |
> 2. s = block_job_create(...); |
> |bdrv_mirror_top_pwritev
> | MirrorBDSOpaque *s = bs->opaque;
> | bool copy_to_target;
> | copy_to_target = s->job->ret >= 0 &&
> | s->job->copy_mode ==
> | MIRROR_COPY_MODE_WRITE_BLOCKING;
> | (s->job is not NULL until 3!)
> 3. bs_opaque->job = s; |
>
> Just moving step 2 & 3 before 1 can avoid this.
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
> ---
> block/mirror.c | 21 ++++++++++-----------
> 1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index e8e8844afc40..7c872be71149 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1600,6 +1600,16 @@ static BlockJob *mirror_start_job(
> mirror_top_bs->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED |
> BDRV_REQ_NO_FALLBACK;
> bs_opaque = g_new0(MirrorBDSOpaque, 1);
> + /* Make sure that the source is not resized while the job is running */
> + s = block_job_create(job_id, driver, NULL, bs,
> + BLK_PERM_CONSISTENT_READ,
> + BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
> + BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD, speed,
> + creation_flags, cb, opaque, errp);
> + if (!s) {
> + goto fail;
> + }
> + bs_opaque->job = s;
> mirror_top_bs->opaque = bs_opaque;
>
> /* bdrv_append takes ownership of the mirror_top_bs reference, need to keep
> @@ -1612,19 +1622,8 @@ static BlockJob *mirror_start_job(
> if (local_err) {
> bdrv_unref(mirror_top_bs);
> error_propagate(errp, local_err);
> - return NULL;
> - }
> -
> - /* Make sure that the source is not resized while the job is running */
> - s = block_job_create(job_id, driver, NULL, mirror_top_bs,
> - BLK_PERM_CONSISTENT_READ,
> - BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
> - BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD, speed,
> - creation_flags, cb, opaque, errp);
> - if (!s) {
> goto fail;
> }
> - bs_opaque->job = s;
>
> /* The block job now has a reference to this node */
> bdrv_unref(mirror_top_bs);
>
On 9/23/2020 10:50 PM, John Snow wrote: > On 8/26/20 9:19 AM, Peng Liang wrote: >> We found an issue when doing block-commit with iothreads, which tries to >> dereference a NULL pointer. >> > > I'm clearing out my patch backlog. I am a bit out of the loop on block > issues at the moment, did this issue get addressed in a later patch that > I am not seeing, or does it still need attention? > > --js > Hi John, I'm very sorry for missing your reply. Michael encountered the problem too and gave a reproducer script. Thanks, Peng
© 2016 - 2024 Red Hat, Inc.