8:19 a.m.
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
8:19 a.m.
New subject: [libvirt] [PATCH for 5.7.0 1/3] qemu_blockjob: Move active commit failed state handling into a function
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(a)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
8:37 a.m.
New subject: [libvirt] [PATCH for 5.7.0 1/3] qemu_blockjob: Move active commit failed state handling into a function
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(a)redhat.com>
---
src/qemu/qemu_blockjob.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
ACK
8:19 a.m.
New subject: [libvirt] [PATCH for 5.7.0 2/3] qemu_blockjob: Print image path on failed security metadata move too
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(a)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
8:37 a.m.
New subject: [libvirt] [PATCH for 5.7.0 2/3] qemu_blockjob: Print image path on failed security metadata move too
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:19 a.m.
New subject: [libvirt] [PATCH for 5.7.0 3/3] qemu_blockjob: Restore seclabels more frequently on job events
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(a)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
8:42 a.m.
New subject: [libvirt] [PATCH for 5.7.0 3/3] qemu_blockjob: Restore seclabels more frequently on job events
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(a)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 :/
9:38 a.m.
New subject: [libvirt] [PATCH for 5.7.0 3/3] qemu_blockjob: Restore seclabels more frequently on job events
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(a)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
1912
days inactive
1912
days old
7 comments
2 participants
participants (2)
-
Michal Privoznik -
Peter Krempa