[libvirt] [PATCH for 5.7.0 0/3] qemu_blockjob: Restore seclabels more frequently
At some points in blockjob code we were not restoring seclabels on backing chain qemu deceased to use. This leads to perms/XATTRs leak problem. 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: Restore seclabels more frequently on job events src/qemu/qemu_blockjob.c | 46 ++++++++++++++++++++++++++++++++++------ 1 file changed, 40 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> --- 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
On Fri, Aug 30, 2019 at 15:19:06 +0200, Michal Privoznik wrote:
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> --- src/qemu/qemu_blockjob.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)
ACK
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> --- src/qemu/qemu_blockjob.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index c77a129bfc..80302fb139 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -646,8 +646,10 @@ 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", + vm->def->name, NULLSTR(disk->src->path), NULLSTR(disk->mirror->path)); + } virObjectUnref(disk->src); disk->src = disk->mirror; -- 2.21.0
On Fri, Aug 30, 2019 at 15:19:07 +0200, Michal Privoznik wrote:
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> --- src/qemu/qemu_blockjob.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
ACK
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index c77a129bfc..80302fb139 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -646,8 +646,10 @@ 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", + vm->def->name, NULLSTR(disk->src->path), NULLSTR(disk->mirror->path));
perhaps also log disk->dst
+ }
virObjectUnref(disk->src); disk->src = disk->mirror; -- 2.21.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
If a block job reaches failed/cancelled state, or is completed without pivot then qemu no longer uses the mirror image. Since we've set its seclabels we must restore them back to avoid leaking perms/XATTRs. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1741456 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_blockjob.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 80302fb139..8411d8e223 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -656,6 +656,13 @@ qemuBlockJobEventProcessLegacyCompleted(virQEMUDriverPtr driver, } else { if (disk->mirror) { virDomainLockImageDetach(driver->lockManager, vm, disk->mirror); + + /* QEMU no longer uses the image, so we can restore its label. */ + if (qemuSecurityRestoreImageLabel(driver, vm, disk->mirror, true) < 0) { + VIR_WARN("Unable to restore security labels on vm %s disk %s", + vm->def->name, NULLSTR(disk->mirror->path)); + } + virObjectUnref(disk->mirror); } } @@ -725,6 +732,13 @@ qemuBlockJobEventProcessLegacy(virQEMUDriverPtr driver, case VIR_DOMAIN_BLOCK_JOB_CANCELED: if (disk->mirror) { virDomainLockImageDetach(driver->lockManager, vm, disk->mirror); + + /* QEMU no longer uses the image, so we can restore its label. */ + if (qemuSecurityRestoreImageLabel(driver, vm, disk->mirror, true) < 0) { + VIR_WARN("Unable to restore security labels on vm %s disk %s", + vm->def->name, NULLSTR(disk->mirror->path)); + } + virObjectUnref(disk->mirror); disk->mirror = NULL; } @@ -1124,7 +1138,8 @@ qemuBlockJobProcessEventConcludedCopyAbort(virQEMUDriverPtr driver, static void -qemuBlockJobProcessEventFailedActiveCommit(virDomainObjPtr vm, +qemuBlockJobProcessEventFailedActiveCommit(virQEMUDriverPtr driver, + virDomainObjPtr vm, qemuBlockJobDataPtr job) { VIR_DEBUG("active commit job '%s' on VM '%s' failed", job->name, vm->def->name); @@ -1132,6 +1147,12 @@ qemuBlockJobProcessEventFailedActiveCommit(virDomainObjPtr vm, if (!job->disk) return; + /* QEMU no longer uses the image, so we can restore its label. */ + if (qemuSecurityRestoreImageLabel(driver, vm, job->disk->mirror, true) < 0) { + VIR_WARN("Unable to restore security labels on vm %s disk %s", + vm->def->name, NULLSTR(job->disk->mirror->path)); + } + virObjectUnref(job->disk->mirror); job->disk->mirror = NULL; } @@ -1227,7 +1248,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 15:19:08 +0200, Michal Privoznik wrote:
If a block job reaches failed/cancelled state, or is completed without pivot then qemu no longer uses the mirror image. Since we've set its seclabels we must restore them back to avoid leaking perms/XATTRs.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1741456
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_blockjob.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 80302fb139..8411d8e223 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -656,6 +656,13 @@ qemuBlockJobEventProcessLegacyCompleted(virQEMUDriverPtr driver, } else { if (disk->mirror) { virDomainLockImageDetach(driver->lockManager, vm, disk->mirror); + + /* QEMU no longer uses the image, so we can restore its label. */ + if (qemuSecurityRestoreImageLabel(driver, vm, disk->mirror, true) < 0) { + VIR_WARN("Unable to restore security labels on vm %s disk %s", + vm->def->name, NULLSTR(disk->mirror->path)); + } +
So unfortunately this is true only for the block copy operation. Active block commit will still continue using the image in read-only mode. Also I'm afraid I remember why this code does not have any security labeling code. The mirror may in fact contain parts of the original backing chain (if the --reuse-external flag was used) and in that case we'd not be able to tell which ones we want to remove labelling from, so the only "safe" operation is to remove labels/locks from the top image only.
virObjectUnref(disk->mirror); } } @@ -725,6 +732,13 @@ qemuBlockJobEventProcessLegacy(virQEMUDriverPtr driver, case VIR_DOMAIN_BLOCK_JOB_CANCELED: if (disk->mirror) { virDomainLockImageDetach(driver->lockManager, vm, disk->mirror); + + /* QEMU no longer uses the image, so we can restore its label. */ + if (qemuSecurityRestoreImageLabel(driver, vm, disk->mirror, true) < 0) { + VIR_WARN("Unable to restore security labels on vm %s disk %s", + vm->def->name, NULLSTR(disk->mirror->path)); + } + virObjectUnref(disk->mirror); disk->mirror = NULL; } @@ -1124,7 +1138,8 @@ qemuBlockJobProcessEventConcludedCopyAbort(virQEMUDriverPtr driver,
static void -qemuBlockJobProcessEventFailedActiveCommit(virDomainObjPtr vm, +qemuBlockJobProcessEventFailedActiveCommit(virQEMUDriverPtr driver, + virDomainObjPtr vm, qemuBlockJobDataPtr job) { VIR_DEBUG("active commit job '%s' on VM '%s' failed", job->name, vm->def->name); @@ -1132,6 +1147,12 @@ qemuBlockJobProcessEventFailedActiveCommit(virDomainObjPtr vm, if (!job->disk) return;
+ /* QEMU no longer uses the image, so we can restore its label. */ + if (qemuSecurityRestoreImageLabel(driver, vm, job->disk->mirror, true) < 0) { + VIR_WARN("Unable to restore security labels on vm %s disk %s", + vm->def->name, NULLSTR(job->disk->mirror->path));
So, here this must return the security labels to readonly state rather than removing it completely as this is still used. I'm sorry these points didn't occur to me when we were discussing this code :/
On 8/30/19 3:42 PM, Peter Krempa wrote:
On Fri, Aug 30, 2019 at 15:19:08 +0200, Michal Privoznik wrote:
If a block job reaches failed/cancelled state, or is completed without pivot then qemu no longer uses the mirror image. Since we've set its seclabels we must restore them back to avoid leaking perms/XATTRs.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1741456
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_blockjob.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-)
@@ -1124,7 +1138,8 @@ qemuBlockJobProcessEventConcludedCopyAbort(virQEMUDriverPtr driver,
static void -qemuBlockJobProcessEventFailedActiveCommit(virDomainObjPtr vm, +qemuBlockJobProcessEventFailedActiveCommit(virQEMUDriverPtr driver, + virDomainObjPtr vm, qemuBlockJobDataPtr job) { VIR_DEBUG("active commit job '%s' on VM '%s' failed", job->name, vm->def->name); @@ -1132,6 +1147,12 @@ qemuBlockJobProcessEventFailedActiveCommit(virDomainObjPtr vm, if (!job->disk) return;
+ /* QEMU no longer uses the image, so we can restore its label. */ + if (qemuSecurityRestoreImageLabel(driver, vm, job->disk->mirror, true) < 0) { + VIR_WARN("Unable to restore security labels on vm %s disk %s", + vm->def->name, NULLSTR(job->disk->mirror->path));
So, here this must return the security labels to readonly state rather than removing it completely as this is still used.
That is not implemented :-( Our virSecuritySELinuxRestoreImageLabel() acts like 'restorecon $path' which is not what we need here. You know what? I'll remove the XATTRs in all three cases (at least for the time being) as it's safe to do so (the worst thing is that we won't restore the original label - but we weren't doing that anyway). For the first two (legacy mode) we can't know if somebody else is not using the backing chain, and in this blockdev case we could know that but that would require bigger fix. Michal
participants (2)
-
Michal Privoznik -
Peter Krempa