There are four places where we remove image XATTRs and in all of them we have the same for() loop with the same body. Move it into a separate function because I'm about to introduce fifth place where the same needs to be done. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_block.c | 25 ++++++++++++++++++++++ src/qemu/qemu_block.h | 6 ++++++ src/qemu/qemu_blockjob.c | 45 ++++------------------------------------ 3 files changed, 35 insertions(+), 41 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index b8f0208742..1140a33114 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -22,6 +22,7 @@ #include "qemu_command.h" #include "qemu_domain.h" #include "qemu_alias.h" +#include "qemu_security.h" #include "viralloc.h" #include "virstring.h" @@ -2588,3 +2589,27 @@ qemuBlockStorageSourceCreateDetectSize(virHashTablePtr blockNamedNodeData, return 0; } + + +int +qemuBlockRemoveImageMetadata(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *diskTarget, + virStorageSourcePtr src) +{ + virStorageSourcePtr n; + int ret = 0; + + for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) { + if (qemuSecurityMoveImageMetadata(driver, vm, n, NULL) < 0) { + VIR_WARN("Unable to remove disk metadata on " + "vm %s from %s (disk target %s)", + vm->def->name, + NULLSTR(n->path), + diskTarget); + ret = -1; + } + } + + return ret; +} diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index 5ddeeaf6e2..5854641027 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -197,3 +197,9 @@ int qemuBlockStorageSourceCreateDetectSize(virHashTablePtr blockNamedNodeData, virStorageSourcePtr src, virStorageSourcePtr templ); + +int +qemuBlockRemoveImageMetadata(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *diskTarget, + virStorageSourcePtr src); diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 5c294f8024..92e4d391c9 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -658,36 +658,18 @@ qemuBlockJobEventProcessLegacyCompleted(virQEMUDriverPtr driver, virObjectUnref(disk->src); disk->src = disk->mirror; } else { - virStorageSourcePtr n; - if (disk->mirror) { virDomainLockImageDetach(driver->lockManager, vm, disk->mirror); /* Ideally, we would restore seclabels on the backing chain here * but we don't know if somebody else is not using parts of it. * Remove security driver metadata so that they are not leaked. */ - for (n = disk->mirror; virStorageSourceIsBacking(n); n = n->backingStore) { - if (qemuSecurityMoveImageMetadata(driver, vm, n, NULL) < 0) { - VIR_WARN("Unable to remove disk metadata on " - "vm %s from %s (disk target %s)", - vm->def->name, - NULLSTR(disk->src->path), - disk->dst); - } - } + qemuBlockRemoveImageMetadata(driver, vm, disk->dst, disk->mirror); virObjectUnref(disk->mirror); } - for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) { - if (qemuSecurityMoveImageMetadata(driver, vm, n, NULL) < 0) { - VIR_WARN("Unable to remove disk metadata on " - "vm %s from %s (disk target %s)", - vm->def->name, - NULLSTR(n->path), - disk->dst); - } - } + qemuBlockRemoveImageMetadata(driver, vm, disk->dst, disk->src); } /* Recompute the cached backing chain to match our @@ -754,22 +736,12 @@ qemuBlockJobEventProcessLegacy(virQEMUDriverPtr driver, case VIR_DOMAIN_BLOCK_JOB_FAILED: case VIR_DOMAIN_BLOCK_JOB_CANCELED: if (disk->mirror) { - virStorageSourcePtr n; - virDomainLockImageDetach(driver->lockManager, vm, disk->mirror); /* Ideally, we would restore seclabels on the backing chain here * but we don't know if somebody else is not using parts of it. * Remove security driver metadata so that they are not leaked. */ - for (n = disk->mirror; virStorageSourceIsBacking(n); n = n->backingStore) { - if (qemuSecurityMoveImageMetadata(driver, vm, n, NULL) < 0) { - VIR_WARN("Unable to remove disk metadata on " - "vm %s from %s (disk target %s)", - vm->def->name, - NULLSTR(disk->src->path), - disk->dst); - } - } + qemuBlockRemoveImageMetadata(driver, vm, disk->dst, disk->mirror); virObjectUnref(disk->mirror); disk->mirror = NULL; @@ -1177,7 +1149,6 @@ qemuBlockJobProcessEventFailedActiveCommit(virQEMUDriverPtr driver, qemuBlockJobDataPtr job) { virDomainDiskDefPtr disk = job->disk; - virStorageSourcePtr n; VIR_DEBUG("active commit job '%s' on VM '%s' failed", job->name, vm->def->name); @@ -1187,15 +1158,7 @@ qemuBlockJobProcessEventFailedActiveCommit(virQEMUDriverPtr driver, /* Ideally, we would make the backing chain read only again (yes, SELinux * can do that using different labels). But that is not implemented yet and * not leaking security driver metadata is more important. */ - for (n = disk->mirror; virStorageSourceIsBacking(n); n = n->backingStore) { - if (qemuSecurityMoveImageMetadata(driver, vm, n, NULL) < 0) { - VIR_WARN("Unable to remove disk metadata on " - "vm %s from %s (disk target %s)", - vm->def->name, - NULLSTR(disk->src->path), - disk->dst); - } - } + qemuBlockRemoveImageMetadata(driver, vm, disk->dst, disk->mirror); virObjectUnref(disk->mirror); disk->mirror = NULL; -- 2.23.0

If user starts a blockcommit without --pivot then we modify access for qemu on both images and leave it like that until pivot is executed. So far so good. Problem is, if user instead of issuing pivot calls destroy on the domain. In this case we don't ever clear the access we granted at the beginning. https://bugzilla.redhat.com/show_bug.cgi?id=1741456#c19 Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_process.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 8cb12ac9a6..fa9167441e 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7617,6 +7617,15 @@ void qemuProcessStop(virQEMUDriverPtr driver, for (i = 0; i < vm->def->niothreadids; i++) vm->def->iothreadids[i]->thread_id = 0; + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr disk = def->disks[i]; + + if (!disk->mirror) + continue; + + qemuBlockRemoveImageMetadata(driver, vm, disk->dst, disk->mirror); + } + /* clear all private data entries which are no longer needed */ qemuDomainObjPrivateDataClear(priv); -- 2.23.0

On Tue, Nov 19, 2019 at 14:15:12 +0100, Michal Privoznik wrote: So aren't we tracking the image metadata also for the shared subsets of backing chain which might be used by multiple domains? Because this undoes everything and we do have some code which does this in the security driver, don't we? So this either is duplicate or it's a superset of what is done in the security driver.

On Wed, Nov 20, 2019 at 15:10:17 +0100, Michal Privoznik wrote: Well, I was worried that we actually call the code even if qemuDomainStorageSourceAccessAllow was never called on the image. For example when no blockjob was ever used. This would be wrong and could actually cut off access for other VMs in case when the image is shared. In case when we run a blockjob we relabel it for write acces but by starting the job the user declares that the chain is not shared in the first place as executing any merge would corrupt other readers. In addition now qemu image locks and our metadata locks should prevent from getting there in the first place, but both of those have some limitations. As mentioned above, the used declared that the chain is not shared by calling the API in the first place but calling it all the time is ... suspicious at best. It feels like the labelling code should handle this if it should be executed all the time during vm shutdown. Since the functions you've pasted above say that it's not the correct thing to do this also must be wrong. If the only correct thing to do is to nuke the xattrs it still feels like the labelling code which put the xattrs there in the first place should deal with it when un-labelling the files and not deal with it explicitly in many places. I can understand that the old block job handlers require this special handling but for the new blockjob handler we unlabel the files as they are being removed so even that place would be fixed with a systemic solution.