[PATCH 0/2] block/export: Fix crash on error after iothread conflict

[PATCH 0/2] block/export: Fix crash on error after iothread conflict

[Max Reitz]

Hi,

By passing the @iothread option to block-export-add, the new export can
be moved to the given iothread.  This may conflict with an existing
parent of the node in question.  How this conflict is resolved, depends
on @fixed-iothread: If that option is true, the error is fatal and
block-export-add fails.  If it is false, the error is ignored and the
node stays in its original iothread.

However, in the implementation, the ignored error is still in *errp, and
so if a second error occurs afterwards and tries to put something into
*errp, that will fail an assertion.

To really ignore the error, we have to free it and clear *errp (with an
ERRP_GUARD()).

Patch 1 is the fix, patch 2 a regression test.


Max Reitz (2):
  block/export: Free ignored Error
  iotests/307: Test iothread conflict for exports

 block/export/export.c      |  4 ++++
 tests/qemu-iotests/307     | 15 +++++++++++++++
 tests/qemu-iotests/307.out |  8 ++++++++
 3 files changed, 27 insertions(+)

-- 
2.30.2

[PATCH 1/2] block/export: Free ignored Error

[Max Reitz]

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;
         }
     }
 
-- 
2.30.2

[PATCH 2/2] iotests/307: Test iothread conflict for exports

[Max Reitz]

Passing fixed-iothread=true should make iothread conflicts fatal,
whereas fixed-iothread=false should not.

Combine the second case with an error condition that is checked after
the iothread is handled, to verify that qemu does not crash if there is
such an error after changing the iothread failed.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/307     | 15 +++++++++++++++
 tests/qemu-iotests/307.out |  8 ++++++++
 2 files changed, 23 insertions(+)

diff --git a/tests/qemu-iotests/307 b/tests/qemu-iotests/307
index c7685347bc..b429b5aa50 100755
--- a/tests/qemu-iotests/307
+++ b/tests/qemu-iotests/307
@@ -41,9 +41,11 @@ with iotests.FilePath('image') as img, \
     iotests.log('=== Launch VM ===')
 
     vm.add_object('iothread,id=iothread0')
+    vm.add_object('iothread,id=iothread1')
     vm.add_blockdev(f'file,filename={img},node-name=file')
     vm.add_blockdev(f'{iotests.imgfmt},file=file,node-name=fmt')
     vm.add_blockdev('raw,file=file,node-name=ro,read-only=on')
+    vm.add_blockdev('null-co,node-name=null')
     vm.add_device(f'id=scsi0,driver=virtio-scsi,iothread=iothread0')
     vm.launch()
 
@@ -74,6 +76,19 @@ with iotests.FilePath('image') as img, \
     vm.qmp_log('query-block-exports')
     iotests.qemu_nbd_list_log('-k', socket)
 
+    iotests.log('\n=== Add export with conflicting iothread ===')
+
+    vm.qmp_log('device_add', id='sdb', driver='scsi-hd', drive='null')
+
+    # Should fail because of fixed-iothread
+    vm.qmp_log('block-export-add', id='export1', type='nbd', node_name='null',
+               iothread='iothread1', fixed_iothread=True, writable=True)
+
+    # Should ignore the iothread conflict, but then fail because of the
+    # permission conflict (and not crash)
+    vm.qmp_log('block-export-add', id='export1', type='nbd', node_name='null',
+               iothread='iothread1', fixed_iothread=False, writable=True)
+
     iotests.log('\n=== Add a writable export ===')
 
     # This fails because share-rw=off
diff --git a/tests/qemu-iotests/307.out b/tests/qemu-iotests/307.out
index daa8ad2da0..69439d96ff 100644
--- a/tests/qemu-iotests/307.out
+++ b/tests/qemu-iotests/307.out
@@ -51,6 +51,14 @@ exports available: 1
    base:allocation
 
 
+=== Add export with conflicting iothread ===
+{"execute": "device_add", "arguments": {"drive": "null", "driver": "scsi-hd", "id": "sdb"}}
+{"return": {}}
+{"execute": "block-export-add", "arguments": {"fixed-iothread": true, "id": "export1", "iothread": "iothread1", "node-name": "null", "type": "nbd", "writable": true}}
+{"error": {"class": "GenericError", "desc": "Cannot change iothread of active block backend"}}
+{"execute": "block-export-add", "arguments": {"fixed-iothread": false, "id": "export1", "iothread": "iothread1", "node-name": "null", "type": "nbd", "writable": true}}
+{"error": {"class": "GenericError", "desc": "Conflicts with use by sdb as 'root', which does not allow 'write' on null"}}
+
 === Add a writable export ===
 {"execute": "block-export-add", "arguments": {"description": "This is the writable second export", "id": "export1", "name": "export1", "node-name": "fmt", "type": "nbd", "writable": true, "writethrough": true}}
 {"error": {"class": "GenericError", "desc": "Conflicts with use by sda as 'root', which does not allow 'write' on fmt"}}
-- 
2.30.2

Re: [PATCH 1/2] block/export: Free ignored Error

[Vladimir Sementsov-Ogievskiy]

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.


-- 
Best regards,
Vladimir

Re: [PATCH 2/2] iotests/307: Test iothread conflict for exports

[Vladimir Sementsov-Ogievskiy]

22.04.2021 17:53, Max Reitz wrote:
> Passing fixed-iothread=true should make iothread conflicts fatal,
> whereas fixed-iothread=false should not.
> 
> Combine the second case with an error condition that is checked after
> the iothread is handled, to verify that qemu does not crash if there is
> such an error after changing the iothread failed.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Tested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir

Re: [PATCH 1/2] block/export: Free ignored Error

[Max Reitz]

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.
> 
> 

Re: [PATCH 1/2] block/export: Free ignored Error

[Vladimir Sementsov-Ogievskiy]

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.
>>
>>
> 


-- 
Best regards,
Vladimir

Re: [PATCH 0/2] block/export: Fix crash on error after iothread conflict

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1222 bytes --]

On Thu, Apr 22, 2021 at 04:53:33PM +0200, Max Reitz wrote:
> Hi,
> 
> By passing the @iothread option to block-export-add, the new export can
> be moved to the given iothread.  This may conflict with an existing
> parent of the node in question.  How this conflict is resolved, depends
> on @fixed-iothread: If that option is true, the error is fatal and
> block-export-add fails.  If it is false, the error is ignored and the
> node stays in its original iothread.
> 
> However, in the implementation, the ignored error is still in *errp, and
> so if a second error occurs afterwards and tries to put something into
> *errp, that will fail an assertion.
> 
> To really ignore the error, we have to free it and clear *errp (with an
> ERRP_GUARD()).
> 
> Patch 1 is the fix, patch 2 a regression test.
> 
> 
> Max Reitz (2):
>   block/export: Free ignored Error
>   iotests/307: Test iothread conflict for exports
> 
>  block/export/export.c      |  4 ++++
>  tests/qemu-iotests/307     | 15 +++++++++++++++
>  tests/qemu-iotests/307.out |  8 ++++++++
>  3 files changed, 27 insertions(+)
> 
> -- 
> 2.30.2
> 

Thanks for fixing this!

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]