[libvirt] [PATCH for 5.7.0 v2 0/3] qemu_blockjob: Remove secdriver metadata more frequently
v2 of: https://www.redhat.com/archives/libvir-list/2019-August/msg01418.html Patches 1/3 and 2/3 were ACKed but I did not pushed them yet - I'd like to push these all at once. diff to v1: - Instead of restoring seclabels on the backing chain, remove the metadata as we can't know if somebody else is not using some parts of the chain. Michal Prívozník (3): qemu_blockjob: Move active commit failed state handling into a function qemu_blockjob: Print image path on failed security metadata move too qemu_blockjob: Remove secdriver metadata more frequently src/qemu/qemu_blockjob.c | 78 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 72 insertions(+), 6 deletions(-) -- 2.21.0
Currently, there are only a few lines of code so a separate function was not necessary, but this will change. So instead of putting all the new code under 'case QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT' create a separate function. Just like every other case has one. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ACKed-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 3003e9c518..c77a129bfc 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -1121,6 +1121,20 @@ qemuBlockJobProcessEventConcludedCopyAbort(virQEMUDriverPtr driver, } +static void +qemuBlockJobProcessEventFailedActiveCommit(virDomainObjPtr vm, + qemuBlockJobDataPtr job) +{ + VIR_DEBUG("active commit job '%s' on VM '%s' failed", job->name, vm->def->name); + + if (!job->disk) + return; + + virObjectUnref(job->disk->mirror); + job->disk->mirror = NULL; +} + + static void qemuBlockJobProcessEventConcludedCreate(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -1211,10 +1225,7 @@ qemuBlockJobEventProcessConcludedTransition(qemuBlockJobDataPtr job, break; case QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT: - if (job->disk) { - virObjectUnref(job->disk->mirror); - job->disk->mirror = NULL; - } + qemuBlockJobProcessEventFailedActiveCommit(vm, job); break; case QEMU_BLOCKJOB_TYPE_CREATE: -- 2.21.0
When a block job is completed, the security image metadata are moved to the new image. If this fails an warning is printed, but the message contains only domain name and lacks image paths. Put them both into the warning message. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ACKed-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index c77a129bfc..1b22689e0c 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -646,8 +646,14 @@ qemuBlockJobEventProcessLegacyCompleted(virQEMUDriverPtr driver, virDomainLockImageDetach(driver->lockManager, vm, disk->src); /* Move secret driver metadata */ - if (qemuSecurityMoveImageMetadata(driver, vm, disk->src, disk->mirror) < 0) - VIR_WARN("Unable to move disk metadata on vm %s", vm->def->name); + if (qemuSecurityMoveImageMetadata(driver, vm, disk->src, disk->mirror) < 0) { + VIR_WARN("Unable to move disk metadata on " + "vm %s from %s to %s (disk target %s)", + vm->def->name, + NULLSTR(disk->src->path), + NULLSTR(disk->mirror->path), + disk->dst); + } virObjectUnref(disk->src); disk->src = disk->mirror; -- 2.21.0
If a block job reaches failed/cancelled state, or is completed without pivot then we must remove security driver metadata associated to the backing chain so that we don't leave any metadata behind. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1741456 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_blockjob.c | 59 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 54 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 1b22689e0c..a991309ee7 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -659,7 +659,23 @@ qemuBlockJobEventProcessLegacyCompleted(virQEMUDriverPtr driver, disk->src = disk->mirror; } else { 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); + } + } + virObjectUnref(disk->mirror); } } @@ -728,7 +744,23 @@ 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); + } + } + virObjectUnref(disk->mirror); disk->mirror = NULL; } @@ -1128,16 +1160,33 @@ qemuBlockJobProcessEventConcludedCopyAbort(virQEMUDriverPtr driver, static void -qemuBlockJobProcessEventFailedActiveCommit(virDomainObjPtr vm, +qemuBlockJobProcessEventFailedActiveCommit(virQEMUDriverPtr driver, + virDomainObjPtr vm, qemuBlockJobDataPtr job) { + virDomainDiskDefPtr disk = job->disk; + virStorageSourcePtr n; + VIR_DEBUG("active commit job '%s' on VM '%s' failed", job->name, vm->def->name); - if (!job->disk) + if (!disk) return; - virObjectUnref(job->disk->mirror); - job->disk->mirror = NULL; + /* 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); + } + } + + virObjectUnref(disk->mirror); + disk->mirror = NULL; } @@ -1231,7 +1280,7 @@ qemuBlockJobEventProcessConcludedTransition(qemuBlockJobDataPtr job, break; case QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT: - qemuBlockJobProcessEventFailedActiveCommit(vm, job); + qemuBlockJobProcessEventFailedActiveCommit(driver, vm, job); break; case QEMU_BLOCKJOB_TYPE_CREATE: -- 2.21.0
On Fri, Aug 30, 2019 at 16:56:10 +0200, Michal Privoznik wrote:
If a block job reaches failed/cancelled state, or is completed without pivot then we must remove security driver metadata associated to the backing chain so that we don't leave any metadata behind.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1741456
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_blockjob.c | 59 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 54 insertions(+), 5 deletions(-)
ACK
participants (2)
-
Michal Privoznik -
Peter Krempa