| Message ID | 20210219153348.41861-4-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | backup-top: Don't crash on post-finalize accesses | expand |
On 19.02.21 16:33, Max Reitz wrote: > Without any of HEAD^ or HEAD^^ applied, qemu will most likely crash on > the qemu-io invocation, for a variety of immediate reasons. The > underlying problem is generally a use-after-free access into > backup-top's BlockCopyState. > > With only HEAD^ applied, qemu-io will run into an EIO (which is not > capture by the output, but you can see that the qemu-io invocation will > be accepted (i.e., qemu-io will run) in contrast to the reference > output, where the node name cannot be found), and qemu will then crash > in query-named-block-nodes: bdrv_get_allocated_file_size() detects > backup-top to be a filter and passes the request through to its child. > However, after bdrv_backup_top_drop(), that child is NULL, so the > recursive call crashes. > > With HEAD^^ applied, this test should pass. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > tests/qemu-iotests/283 | 55 ++++++++++++++++++++++++++++++++++++++ > tests/qemu-iotests/283.out | 15 +++++++++++ > 2 files changed, 70 insertions(+) > > diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283 > index 79643e375b..509dcbbcf4 100755 > --- a/tests/qemu-iotests/283 > +++ b/tests/qemu-iotests/283 > @@ -97,3 +97,58 @@ vm.qmp_log('blockdev-add', **{ > vm.qmp_log('blockdev-backup', sync='full', device='source', target='target') > > vm.shutdown() > + > + > +""" > +Check that the backup-top node is gone after job-finalize. > + > +During finalization, the node becomes inactive and can no longer > +function. If it is still present, new parents might be attached, and > +there would be no meaningful way to handle their I/O requests. > +""" Oh no, 297/pylint complains that this “string statement has no effect”. Guess it should be a normal comment under the following print() then... Max > +print('\n=== backup-top should be gone after job-finalize ===\n')
Am 19.02.2021 um 16:59 hat Max Reitz geschrieben: > On 19.02.21 16:33, Max Reitz wrote: > > Without any of HEAD^ or HEAD^^ applied, qemu will most likely crash on > > the qemu-io invocation, for a variety of immediate reasons. The > > underlying problem is generally a use-after-free access into > > backup-top's BlockCopyState. > > > > With only HEAD^ applied, qemu-io will run into an EIO (which is not > > capture by the output, but you can see that the qemu-io invocation will > > be accepted (i.e., qemu-io will run) in contrast to the reference > > output, where the node name cannot be found), and qemu will then crash > > in query-named-block-nodes: bdrv_get_allocated_file_size() detects > > backup-top to be a filter and passes the request through to its child. > > However, after bdrv_backup_top_drop(), that child is NULL, so the > > recursive call crashes. > > > > With HEAD^^ applied, this test should pass. > > > > Signed-off-by: Max Reitz <mreitz@redhat.com> > > --- > > tests/qemu-iotests/283 | 55 ++++++++++++++++++++++++++++++++++++++ > > tests/qemu-iotests/283.out | 15 +++++++++++ > > 2 files changed, 70 insertions(+) > > > > diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283 > > index 79643e375b..509dcbbcf4 100755 > > --- a/tests/qemu-iotests/283 > > +++ b/tests/qemu-iotests/283 > > @@ -97,3 +97,58 @@ vm.qmp_log('blockdev-add', **{ > > vm.qmp_log('blockdev-backup', sync='full', device='source', target='target') > > vm.shutdown() > > + > > + > > +""" > > +Check that the backup-top node is gone after job-finalize. > > + > > +During finalization, the node becomes inactive and can no longer > > +function. If it is still present, new parents might be attached, and > > +there would be no meaningful way to handle their I/O requests. > > +""" > > Oh no, 297/pylint complains that this “string statement has no effect”. > Guess it should be a normal comment under the following print() then... Thanks, fixed up the comment as you suggest and applied to the block branch. Kevin
diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283 index 79643e375b..509dcbbcf4 100755 --- a/tests/qemu-iotests/283 +++ b/tests/qemu-iotests/283 @@ -97,3 +97,58 @@ vm.qmp_log('blockdev-add', **{ vm.qmp_log('blockdev-backup', sync='full', device='source', target='target') vm.shutdown() + + +""" +Check that the backup-top node is gone after job-finalize. + +During finalization, the node becomes inactive and can no longer +function. If it is still present, new parents might be attached, and +there would be no meaningful way to handle their I/O requests. +""" + +print('\n=== backup-top should be gone after job-finalize ===\n') + +vm = iotests.VM() +vm.launch() + +vm.qmp_log('blockdev-add', **{ + 'node-name': 'source', + 'driver': 'null-co', +}) + +vm.qmp_log('blockdev-add', **{ + 'node-name': 'target', + 'driver': 'null-co', +}) + +vm.qmp_log('blockdev-backup', + job_id='backup', + device='source', + target='target', + sync='full', + filter_node_name='backup-filter', + auto_finalize=False, + auto_dismiss=False) + +vm.event_wait('BLOCK_JOB_PENDING', 5.0) + +# The backup-top filter should still be present prior to finalization +assert vm.node_info('backup-filter') is not None + +vm.qmp_log('job-finalize', id='backup') +vm.event_wait('BLOCK_JOB_COMPLETED', 5.0) + +# The filter should be gone now. Check that by trying to access it +# with qemu-io (which will most likely crash qemu if it is still +# there.). +vm.qmp_log('human-monitor-command', + command_line='qemu-io backup-filter "write 0 1M"') + +# (Also, do an explicit check.) +assert vm.node_info('backup-filter') is None + +vm.qmp_log('job-dismiss', id='backup') +vm.event_wait('JOB_STATUS_CHANGE', 5.0, {'data': {'status': 'null'}}) + +vm.shutdown() diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out index d8cff22cc1..7e9cd9a7d4 100644 --- a/tests/qemu-iotests/283.out +++ b/tests/qemu-iotests/283.out @@ -6,3 +6,18 @@ {"return": {}} {"execute": "blockdev-backup", "arguments": {"device": "source", "sync": "full", "target": "target"}} {"error": {"class": "GenericError", "desc": "Cannot set permissions for backup-top filter: Conflicts with use by other as 'image', which uses 'write' on base"}} + +=== backup-top should be gone after job-finalize === + +{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "source"}} +{"return": {}} +{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "target"}} +{"return": {}} +{"execute": "blockdev-backup", "arguments": {"auto-dismiss": false, "auto-finalize": false, "device": "source", "filter-node-name": "backup-filter", "job-id": "backup", "sync": "full", "target": "target"}} +{"return": {}} +{"execute": "job-finalize", "arguments": {"id": "backup"}} +{"return": {}} +{"execute": "human-monitor-command", "arguments": {"command-line": "qemu-io backup-filter \"write 0 1M\""}} +{"return": "Error: Cannot find device= nor node_name=backup-filter\r\n"} +{"execute": "job-dismiss", "arguments": {"id": "backup"}} +{"return": {}}
Without any of HEAD^ or HEAD^^ applied, qemu will most likely crash on the qemu-io invocation, for a variety of immediate reasons. The underlying problem is generally a use-after-free access into backup-top's BlockCopyState. With only HEAD^ applied, qemu-io will run into an EIO (which is not capture by the output, but you can see that the qemu-io invocation will be accepted (i.e., qemu-io will run) in contrast to the reference output, where the node name cannot be found), and qemu will then crash in query-named-block-nodes: bdrv_get_allocated_file_size() detects backup-top to be a filter and passes the request through to its child. However, after bdrv_backup_top_drop(), that child is NULL, so the recursive call crashes. With HEAD^^ applied, this test should pass. Signed-off-by: Max Reitz <mreitz@redhat.com> --- tests/qemu-iotests/283 | 55 ++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/283.out | 15 +++++++++++ 2 files changed, 70 insertions(+)