[PATCH] qemu: Revoke access to mirror on failed blockcopy
When preparing to do a blockcopy, the mirror image is modified so that QEMU can access it. For instance, the mirror has seclabels set, if it is a NVMe disk it is detached from the host and so on. And usually, the restore is done upon successful finish of the blockcopy operation. But, if something fails then we need to explicitly revoke the access to the mirror image (and thus reattach NVMe disk back to the host). Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1822538 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 31f199fdef..9475235f01 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17950,6 +17950,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, virDomainDiskDefPtr disk = NULL; int ret = -1; bool need_unlink = false; + bool need_revoke = false; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); const char *format = NULL; bool mirror_reuse = !!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT); @@ -18124,6 +18125,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, if (qemuDomainStorageSourceChainAccessAllow(driver, vm, mirror) < 0) goto endjob; + need_revoke = true; if (blockdev) { if (mirror_reuse) { @@ -18240,6 +18242,8 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, if (crdata) qemuBlockStorageSourceAttachRollback(priv->mon, crdata->srcdata[0]); ignore_value(qemuDomainObjExitMonitor(driver, vm)); + if (need_revoke) + qemuDomainStorageSourceChainAccessRevoke(driver, vm, mirror); } if (need_unlink && virStorageFileUnlink(mirror) < 0) VIR_WARN("%s", _("unable to remove just-created copy target")); -- 2.24.1
On 4/14/20 6:32 AM, Michal Privoznik wrote:
When preparing to do a blockcopy, the mirror image is modified so that QEMU can access it. For instance, the mirror has seclabels set, if it is a NVMe disk it is detached from the host and so on. And usually, the restore is done upon successful finish of the blockcopy operation. But, if something fails then we need to explicitly revoke the access to the mirror image (and thus reattach NVMe disk back to the host).
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1822538
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
On Tue, Apr 14, 2020 at 11:32:08AM +0200, Michal Privoznik wrote:
When preparing to do a blockcopy, the mirror image is modified so that QEMU can access it. For instance, the mirror has seclabels set, if it is a NVMe disk it is detached from the host and so on. And usually, the restore is done upon successful finish of the blockcopy operation. But, if something fails then we need to explicitly revoke the access to the mirror image (and thus reattach NVMe disk back to the host).
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1822538
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 31f199fdef..9475235f01 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17950,6 +17950,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, virDomainDiskDefPtr disk = NULL; int ret = -1; bool need_unlink = false; + bool need_revoke = false; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); const char *format = NULL; bool mirror_reuse = !!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT); @@ -18124,6 +18125,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
if (qemuDomainStorageSourceChainAccessAllow(driver, vm, mirror) < 0) goto endjob; + need_revoke = true;
if (blockdev) { if (mirror_reuse) { @@ -18240,6 +18242,8 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, if (crdata) qemuBlockStorageSourceAttachRollback(priv->mon, crdata->srcdata[0]); ignore_value(qemuDomainObjExitMonitor(driver, vm)); + if (need_revoke) + qemuDomainStorageSourceChainAccessRevoke(driver, vm, mirror); } if (need_unlink && virStorageFileUnlink(mirror) < 0) VIR_WARN("%s", _("unable to remove just-created copy target"));
Is the revocation code correctly placed though? I mean, it seems to follow from the patch that we need to revoke as soon as qemuDomainStorageSourceChainAccessAllow() succeeds. However, the revocation call is conditioned by there being 'data' or 'crdata' (among other things). What happens if qemuDomainStorageSourceChainAccessAllow() succeeds but the first subsequent attempt to retrieve 'data' or 'crdata' (whichever comes first) fails? These failures are handled by 'goto endjob' which reaches the clean-up section apparently with both 'data' and 'crdata' still being NULL but 'need_revoke' true. If the above is correct, that would mean the 'if (need_revoke)' code is never reached and no revocation performed. What am I missing? pvl
On 4/15/20 2:09 PM, Pavel Mores wrote:
On Tue, Apr 14, 2020 at 11:32:08AM +0200, Michal Privoznik wrote:
When preparing to do a blockcopy, the mirror image is modified so that QEMU can access it. For instance, the mirror has seclabels set, if it is a NVMe disk it is detached from the host and so on. And usually, the restore is done upon successful finish of the blockcopy operation. But, if something fails then we need to explicitly revoke the access to the mirror image (and thus reattach NVMe disk back to the host).
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1822538
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 31f199fdef..9475235f01 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17950,6 +17950,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, virDomainDiskDefPtr disk = NULL; int ret = -1; bool need_unlink = false; + bool need_revoke = false; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); const char *format = NULL; bool mirror_reuse = !!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT); @@ -18124,6 +18125,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
if (qemuDomainStorageSourceChainAccessAllow(driver, vm, mirror) < 0) goto endjob; + need_revoke = true;
if (blockdev) { if (mirror_reuse) { @@ -18240,6 +18242,8 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, if (crdata) qemuBlockStorageSourceAttachRollback(priv->mon, crdata->srcdata[0]); ignore_value(qemuDomainObjExitMonitor(driver, vm)); + if (need_revoke) + qemuDomainStorageSourceChainAccessRevoke(driver, vm, mirror); } if (need_unlink && virStorageFileUnlink(mirror) < 0) VIR_WARN("%s", _("unable to remove just-created copy target"));
Is the revocation code correctly placed though? I mean, it seems to follow from the patch that we need to revoke as soon as qemuDomainStorageSourceChainAccessAllow() succeeds. However, the revocation call is conditioned by there being 'data' or 'crdata' (among other things).
What happens if qemuDomainStorageSourceChainAccessAllow() succeeds but the first subsequent attempt to retrieve 'data' or 'crdata' (whichever comes first) fails? These failures are handled by 'goto endjob' which reaches the clean-up section apparently with both 'data' and 'crdata' still being NULL but 'need_revoke' true. If the above is correct, that would mean the 'if (need_revoke)' code is never reached and no revocation performed.
What am I missing?
Nothing, my patch is broken. I will send v2. Michal
participants (3)
-
Daniel Henrique Barboza -
Michal Privoznik -
Pavel Mores