qemu_blockjob: Remove image metadata on blockcommit

[libvirt] [PATCH] qemu_blockjob: Remove image metadata on blockcommit

Posted by Michal Privoznik 4 years, 7 months ago

When doing a blockcommit, the base and possibly top parent are
relabelled in qemuDomainBlockCommit(). However, once the block
job is finished, we need to remove the secdriver metadata
(XATTRs) created at the beginning.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1741456

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_blockjob.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index a991309ee7..47715f12f6 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -677,6 +677,25 @@ qemuBlockJobEventProcessLegacyCompleted(virQEMUDriverPtr driver,
             }
 
             virObjectUnref(disk->mirror);
+        } else if (job->type == QEMU_BLOCKJOB_TYPE_COMMIT) {
+            if (qemuSecurityMoveImageMetadata(driver, vm,
+                                              job->data.commit.base, NULL) < 0) {
+                VIR_WARN("Unable to remove disk metadata on "
+                         "vm %s from %s (disk target %s)",
+                         vm->def->name,
+                         NULLSTR(job->data.commit.base->path),
+                         disk->dst);
+            }
+            if (job->data.commit.topparent &&
+                job->data.commit.topparent != disk->src &&
+                qemuSecurityMoveImageMetadata(driver, vm,
+                                              job->data.commit.topparent, NULL) < 0) {
+                VIR_WARN("Unable to remove disk metadata on "
+                         "vm %s from %s (disk target %s)",
+                         vm->def->name,
+                         NULLSTR(job->data.commit.topparent->path),
+                         disk->dst);
+            }
         }
     }
 
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu_blockjob: Remove image metadata on blockcommit

Posted by Peter Krempa 4 years, 7 months ago

On Mon, Sep 16, 2019 at 08:49:37 +0200, Michal Privoznik wrote:
> When doing a blockcommit, the base and possibly top parent are
> relabelled in qemuDomainBlockCommit(). However, once the block
> job is finished, we need to remove the secdriver metadata
> (XATTRs) created at the beginning.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1741456
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_blockjob.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
> index a991309ee7..47715f12f6 100644
> --- a/src/qemu/qemu_blockjob.c
> +++ b/src/qemu/qemu_blockjob.c
> @@ -677,6 +677,25 @@ qemuBlockJobEventProcessLegacyCompleted(virQEMUDriverPtr driver,
>              }
>  
>              virObjectUnref(disk->mirror);
> +        } else if (job->type == QEMU_BLOCKJOB_TYPE_COMMIT) {
> +            if (qemuSecurityMoveImageMetadata(driver, vm,
> +                                              job->data.commit.base, NULL) < 0) {
> +                VIR_WARN("Unable to remove disk metadata on "
> +                         "vm %s from %s (disk target %s)",
> +                         vm->def->name,
> +                         NULLSTR(job->data.commit.base->path),
> +                         disk->dst);
> +            }
> +            if (job->data.commit.topparent &&
> +                job->data.commit.topparent != disk->src &&
> +                qemuSecurityMoveImageMetadata(driver, vm,
> +                                              job->data.commit.topparent, NULL) < 0) {
> +                VIR_WARN("Unable to remove disk metadata on "
> +                         "vm %s from %s (disk target %s)",
> +                         vm->def->name,
> +                         NULLSTR(job->data.commit.topparent->path),
> +                         disk->dst);
> +            }

NACK, the legacy block job handler was never designed to touch job->data
in any way. I don't want to guarantee that the blockjob code bits work
with the legacy code bits in any way.

Specifically the problem is that any of this data will NOT be available
after libvirtd restart because all of the job tracking in the status XML
is only done when QEMU_CAPS_BLOCKDEV is enabled.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu_blockjob: Remove image metadata on blockcommit

Posted by Michal Privoznik 4 years, 7 months ago

On 9/16/19 9:40 AM, Peter Krempa wrote:
> On Mon, Sep 16, 2019 at 08:49:37 +0200, Michal Privoznik wrote:
>> When doing a blockcommit, the base and possibly top parent are
>> relabelled in qemuDomainBlockCommit(). However, once the block
>> job is finished, we need to remove the secdriver metadata
>> (XATTRs) created at the beginning.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1741456
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>   src/qemu/qemu_blockjob.c | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>>
>> diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
>> index a991309ee7..47715f12f6 100644
>> --- a/src/qemu/qemu_blockjob.c
>> +++ b/src/qemu/qemu_blockjob.c
>> @@ -677,6 +677,25 @@ qemuBlockJobEventProcessLegacyCompleted(virQEMUDriverPtr driver,
>>               }
>>   
>>               virObjectUnref(disk->mirror);
>> +        } else if (job->type == QEMU_BLOCKJOB_TYPE_COMMIT) {
>> +            if (qemuSecurityMoveImageMetadata(driver, vm,
>> +                                              job->data.commit.base, NULL) < 0) {
>> +                VIR_WARN("Unable to remove disk metadata on "
>> +                         "vm %s from %s (disk target %s)",
>> +                         vm->def->name,
>> +                         NULLSTR(job->data.commit.base->path),
>> +                         disk->dst);
>> +            }
>> +            if (job->data.commit.topparent &&
>> +                job->data.commit.topparent != disk->src &&
>> +                qemuSecurityMoveImageMetadata(driver, vm,
>> +                                              job->data.commit.topparent, NULL) < 0) {
>> +                VIR_WARN("Unable to remove disk metadata on "
>> +                         "vm %s from %s (disk target %s)",
>> +                         vm->def->name,
>> +                         NULLSTR(job->data.commit.topparent->path),
>> +                         disk->dst);
>> +            }
> 
> NACK, the legacy block job handler was never designed to touch job->data
> in any way. I don't want to guarantee that the blockjob code bits work
> with the legacy code bits in any way.
> 
> Specifically the problem is that any of this data will NOT be available
> after libvirtd restart because all of the job tracking in the status XML
> is only done when QEMU_CAPS_BLOCKDEV is enabled.
> 

So how do you suggest I fix this? Steps to reproduce are here:

https://bugzilla.redhat.com/show_bug.cgi?id=1741456#c8

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu_blockjob: Remove image metadata on blockcommit

Posted by Peter Krempa 4 years, 7 months ago

On Mon, Sep 16, 2019 at 09:52:28 +0200, Michal Privoznik wrote:
> On 9/16/19 9:40 AM, Peter Krempa wrote:
> > On Mon, Sep 16, 2019 at 08:49:37 +0200, Michal Privoznik wrote:
> > > When doing a blockcommit, the base and possibly top parent are
> > > relabelled in qemuDomainBlockCommit(). However, once the block
> > > job is finished, we need to remove the secdriver metadata
> > > (XATTRs) created at the beginning.
> > > 
> > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1741456
> > > 
> > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > > ---
> > >   src/qemu/qemu_blockjob.c | 19 +++++++++++++++++++
> > >   1 file changed, 19 insertions(+)
> > > 
> > > diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
> > > index a991309ee7..47715f12f6 100644
> > > --- a/src/qemu/qemu_blockjob.c
> > > +++ b/src/qemu/qemu_blockjob.c
> > > @@ -677,6 +677,25 @@ qemuBlockJobEventProcessLegacyCompleted(virQEMUDriverPtr driver,
> > >               }
> > >               virObjectUnref(disk->mirror);
> > > +        } else if (job->type == QEMU_BLOCKJOB_TYPE_COMMIT) {
> > > +            if (qemuSecurityMoveImageMetadata(driver, vm,
> > > +                                              job->data.commit.base, NULL) < 0) {
> > > +                VIR_WARN("Unable to remove disk metadata on "
> > > +                         "vm %s from %s (disk target %s)",
> > > +                         vm->def->name,
> > > +                         NULLSTR(job->data.commit.base->path),
> > > +                         disk->dst);
> > > +            }
> > > +            if (job->data.commit.topparent &&
> > > +                job->data.commit.topparent != disk->src &&
> > > +                qemuSecurityMoveImageMetadata(driver, vm,
> > > +                                              job->data.commit.topparent, NULL) < 0) {
> > > +                VIR_WARN("Unable to remove disk metadata on "
> > > +                         "vm %s from %s (disk target %s)",
> > > +                         vm->def->name,
> > > +                         NULLSTR(job->data.commit.topparent->path),
> > > +                         disk->dst);
> > > +            }
> > 
> > NACK, the legacy block job handler was never designed to touch job->data
> > in any way. I don't want to guarantee that the blockjob code bits work
> > with the legacy code bits in any way.
> > 
> > Specifically the problem is that any of this data will NOT be available
> > after libvirtd restart because all of the job tracking in the status XML
> > is only done when QEMU_CAPS_BLOCKDEV is enabled.
> > 
> 
> So how do you suggest I fix this? Steps to reproduce are here:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1741456#c8

Historically the legacy block job handler leaked all permissions it
granted to any image during finishing of the block job.

In this case since an action must be taken toI suggest discarding all
metadata on the backing chain of disk->src. That data is valid because
it is refreshed from the disk on VM startup.

While I'd prefer if the legacy handler is not touched any more, removing
all xattrs here is an acceptable fix.

Obviously the second solution is to ensure that the image metadata is
handled only when libvirt knows how to manage the backing chain, thus
enabling it only if QEMU_CAPS_BLOCKDEV is enabled.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list