| Message ID | 20210422145335.65814-2-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block/export: Fix crash on error after iothread conflict | expand |
22.04.2021 17:53, Max Reitz wrote: > When invoking block-export-add with some iothread and > fixed-iothread=false, and changing the node's iothread fails, the error > is supposed to be ignored. > > However, it is still stored in *errp, which is wrong. If a second error > occurs, the "*errp must be NULL" assertion in error_setv() fails: > > qemu-system-x86_64: ../util/error.c:59: error_setv: Assertion > `*errp == NULL' failed. > > So the error from bdrv_try_set_aio_context() must be freed when it is > ignored. > > Fixes: f51d23c80af73c95e0ce703ad06a300f1b3d63ef > ("block/export: add iothread and fixed-iothread options") > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/export/export.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/block/export/export.c b/block/export/export.c > index fec7d9f738..ce5dd3e59b 100644 > --- a/block/export/export.c > +++ b/block/export/export.c > @@ -68,6 +68,7 @@ static const BlockExportDriver *blk_exp_find_driver(BlockExportType type) > > BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp) > { > + ERRP_GUARD(); > bool fixed_iothread = export->has_fixed_iothread && export->fixed_iothread; > const BlockExportDriver *drv; > BlockExport *exp = NULL; > @@ -127,6 +128,9 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp) > ctx = new_ctx; > } else if (fixed_iothread) { > goto fail; > + } else { > + error_free(*errp); > + *errp = NULL; > } > } > > I don't think ERRP_GUARD is needed in this case: we don't need to handle errp somehow except for just free if it was set. So we can simply do: } else if (errp) { error_free(*errp); *errp = NULL; } Let's only check that errp is really set on failure path of bdrv_try_set_aio_context(): bdrv_try_set_aio_context() fails iff bdrv_can_set_aio_context() fails, which in turn may fail iff bdrv_parent_can_set_aio_context() or bdrv_child_can_set_aio_context() fails. bdrv_parent_can_set_aio_context() has two failure path, on first it set errp by hand, and on second it has assertion that errp is set. bdrv_child_can_set_aio_context() may fail only if nested call to bdrv_can_set_aio_context() fails, so recursion is closed.
On 26.04.21 11:44, Vladimir Sementsov-Ogievskiy wrote: > 22.04.2021 17:53, Max Reitz wrote: >> When invoking block-export-add with some iothread and >> fixed-iothread=false, and changing the node's iothread fails, the error >> is supposed to be ignored. >> >> However, it is still stored in *errp, which is wrong. If a second error >> occurs, the "*errp must be NULL" assertion in error_setv() fails: >> >> qemu-system-x86_64: ../util/error.c:59: error_setv: Assertion >> `*errp == NULL' failed. >> >> So the error from bdrv_try_set_aio_context() must be freed when it is >> ignored. >> >> Fixes: f51d23c80af73c95e0ce703ad06a300f1b3d63ef >> ("block/export: add iothread and fixed-iothread options") >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block/export/export.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/block/export/export.c b/block/export/export.c >> index fec7d9f738..ce5dd3e59b 100644 >> --- a/block/export/export.c >> +++ b/block/export/export.c >> @@ -68,6 +68,7 @@ static const BlockExportDriver >> *blk_exp_find_driver(BlockExportType type) >> BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp) >> { >> + ERRP_GUARD(); >> bool fixed_iothread = export->has_fixed_iothread && >> export->fixed_iothread; >> const BlockExportDriver *drv; >> BlockExport *exp = NULL; >> @@ -127,6 +128,9 @@ BlockExport *blk_exp_add(BlockExportOptions >> *export, Error **errp) >> ctx = new_ctx; >> } else if (fixed_iothread) { >> goto fail; >> + } else { >> + error_free(*errp); >> + *errp = NULL; >> } >> } >> > > I don't think ERRP_GUARD is needed in this case: we don't need to handle > errp somehow except for just free if it was set. Perhaps not, but style-wise, I prefer not special-casing the errp == NULL case. (It can be argued that ERRP_GUARD similarly special-cases it, but that’s hidden from my view. Also, the errp == NULL case actually doesn’t even happen, so ERRP_GUARD is effectively a no-op and it won’t cost performance (not that it really matters).) Of course we could also do this: ret = bdrv_try_set_aio_context(bs, new_ctx, fixed_iothread ? errp : NULL); Would be even shorter. > So we can simply do: > > } else if (errp) { > error_free(*errp); > *errp = NULL; > } > > Let's only check that errp is really set on failure path of > bdrv_try_set_aio_context(): OK, but out of interest, why? error_free() doesn’t care. I mean it might be a problem if blk_exp_add() returns an error without setting *errp, but that’d’ve been pre-existing. Max > bdrv_try_set_aio_context() fails iff bdrv_can_set_aio_context() fails, > which in turn may fail iff bdrv_parent_can_set_aio_context() or > bdrv_child_can_set_aio_context() fails. > > bdrv_parent_can_set_aio_context() has two failure path, on first it set > errp by hand, and on second it has assertion that errp is set. > > bdrv_child_can_set_aio_context() may fail only if nested call to > bdrv_can_set_aio_context() fails, so recursion is closed. > >
26.04.2021 13:33, Max Reitz wrote: > On 26.04.21 11:44, Vladimir Sementsov-Ogievskiy wrote: >> 22.04.2021 17:53, Max Reitz wrote: >>> When invoking block-export-add with some iothread and >>> fixed-iothread=false, and changing the node's iothread fails, the error >>> is supposed to be ignored. >>> >>> However, it is still stored in *errp, which is wrong. If a second error >>> occurs, the "*errp must be NULL" assertion in error_setv() fails: >>> >>> qemu-system-x86_64: ../util/error.c:59: error_setv: Assertion >>> `*errp == NULL' failed. >>> >>> So the error from bdrv_try_set_aio_context() must be freed when it is >>> ignored. >>> >>> Fixes: f51d23c80af73c95e0ce703ad06a300f1b3d63ef >>> ("block/export: add iothread and fixed-iothread options") >>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>> --- >>> block/export/export.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/block/export/export.c b/block/export/export.c >>> index fec7d9f738..ce5dd3e59b 100644 >>> --- a/block/export/export.c >>> +++ b/block/export/export.c >>> @@ -68,6 +68,7 @@ static const BlockExportDriver *blk_exp_find_driver(BlockExportType type) >>> BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp) >>> { >>> + ERRP_GUARD(); >>> bool fixed_iothread = export->has_fixed_iothread && export->fixed_iothread; >>> const BlockExportDriver *drv; >>> BlockExport *exp = NULL; >>> @@ -127,6 +128,9 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp) >>> ctx = new_ctx; >>> } else if (fixed_iothread) { >>> goto fail; >>> + } else { >>> + error_free(*errp); >>> + *errp = NULL; >>> } >>> } >>> >> >> I don't think ERRP_GUARD is needed in this case: we don't need to handle errp somehow except for just free if it was set. > > Perhaps not, but style-wise, I prefer not special-casing the > errp == NULL case. > > (It can be argued that ERRP_GUARD similarly special-cases it, but that’s hidden from my view. Also, the errp == NULL case actually doesn’t even happen, so ERRP_GUARD is effectively a no-op and it won’t cost performance (not that it really matters).) Hm. I don't know. May be you are right.. Actually, I don't care too much, so, patch is OK as is: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > Of course we could also do this: > > ret = bdrv_try_set_aio_context(bs, new_ctx, fixed_iothread ? errp : NULL); > > Would be even shorter. > >> So we can simply do: >> >> } else if (errp) { >> error_free(*errp); >> *errp = NULL; >> } >> >> Let's only check that errp is really set on failure path of bdrv_try_set_aio_context(): > > OK, but out of interest, why? error_free() doesn’t care. I mean it might be a problem if blk_exp_add() returns an error without setting *errp, but that’d’ve been pre-existing. > I remember we still have some functions not setting errp on some error paths.. bdrv_open_driver() has work-around for such bad .*open handlers of some drivers... So I decided to look through. > >> bdrv_try_set_aio_context() fails iff bdrv_can_set_aio_context() fails, which in turn may fail iff bdrv_parent_can_set_aio_context() or bdrv_child_can_set_aio_context() fails. >> >> bdrv_parent_can_set_aio_context() has two failure path, on first it set errp by hand, and on second it has assertion that errp is set. >> >> bdrv_child_can_set_aio_context() may fail only if nested call to bdrv_can_set_aio_context() fails, so recursion is closed. >> >> >
diff --git a/block/export/export.c b/block/export/export.c index fec7d9f738..ce5dd3e59b 100644 --- a/block/export/export.c +++ b/block/export/export.c @@ -68,6 +68,7 @@ static const BlockExportDriver *blk_exp_find_driver(BlockExportType type) BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp) { + ERRP_GUARD(); bool fixed_iothread = export->has_fixed_iothread && export->fixed_iothread; const BlockExportDriver *drv; BlockExport *exp = NULL; @@ -127,6 +128,9 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp) ctx = new_ctx; } else if (fixed_iothread) { goto fail; + } else { + error_free(*errp); + *errp = NULL; } }
When invoking block-export-add with some iothread and fixed-iothread=false, and changing the node's iothread fails, the error is supposed to be ignored. However, it is still stored in *errp, which is wrong. If a second error occurs, the "*errp must be NULL" assertion in error_setv() fails: qemu-system-x86_64: ../util/error.c:59: error_setv: Assertion `*errp == NULL' failed. So the error from bdrv_try_set_aio_context() must be freed when it is ignored. Fixes: f51d23c80af73c95e0ce703ad06a300f1b3d63ef ("block/export: add iothread and fixed-iothread options") Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/export/export.c | 4 ++++ 1 file changed, 4 insertions(+)