[PATCH] Don't require secdrivers to implement .domainMoveImageMetadata

[Christian Ehrhardt]

On Mon, May 18, 2020 at 10:07 AM Michal Privoznik <mprivozn at redhat.com> wrote:
>
> On 5/18/20 10:02 AM, Erik Skultety wrote:
>
> > Yes, I know, what I meant by "unrelated" here was just the fact that in order
> > to fix Apparmor, you only need the first hunk, I guess I'll be more explicit
> > next time :). It's true that with the first hunk the second becomes redundant,
> > but I still feel like the NOP driver should cover the full spectrum of
> > operations we support, but maybe I'm trying to be overly cautious here.
> >
>
> Well, it doesn't implement everything already. But okay, I have ACK for
> the important hunk so I will push only that one.

Hi Michal,
while debugging a Ubuntu bug report I found that this fix will
mitigate the warning you wanted to fix.
But overall there still is an issue with the labeling introduced by
[1][2] in >=5.6.

The situation (related to this fix, hence replying in this context) is
the following.
Ubuntu (as an example) builds and has build with --without-attr.

That will make the code have !HAVE_LIBATTR which was fine in the past.
But with your code added the LP bug [3] identified an issue.

What happens is that in stacked security it will iterate on:
- virSecurityStackMoveImageMetadata (for the stacking itself)
- 0x0 (apparmor)
- virSecurityDACMoveImageMetadata

The fix discussed here fixes the warning emitted by the apparmor case like:
"this function is not supported by the connection driver"

But when iterating further on a build that has no attr support we
encounter the following (e.g. at guest shutdown):

libvirtd[6320]: internal error: child reported (status=125):
libvirtd[6320]: Unable to remove disk metadata on vm testguest from
/var/lib/uvtool/libvirt/images/testguest.qcow (disk target vda)

I found that this is due to:
qemuBlockRemoveImageMetadata (the one that emits the warning)
  -> qemuSecurityMoveImageMetadata
    -> virSecurityManagerMoveImageMetadata
      -> virSecurityDACMoveImageMetadata
        -> virSecurityDACMoveImageMetadataHelper
          -> virProcessRunInFork (spawns subprocess)
            -> virSecurityMoveRememberedLabel

Since this is built without HAVE_LIBATTR the following will happen

461 if (virFileGetXAttrQuiet(src, ref_name, &ref_value) < 0) {
(gdb) n
462 if (errno == ENOSYS || errno == ENOTSUP) {
(gdb) p errno
$32 = 38

And that is due to !HAVE_LIBATTR which maps the implementation onto:

4412 #else /* !HAVE_LIBATTR */
4413
4414 int
4415 virFileGetXAttrQuiet(const char *path G_GNUC_UNUSED,
4416 const char *name G_GNUC_UNUSED,
4417 char **value G_GNUC_UNUSED)
4418 {
4419 errno = ENOSYS;
4420 return -1;
4421 }

Due to that we see the two messages reported above
a) internal errors -> for the subprocess that failed
b) "Unable to remove disk metadata" -> but this time due to DAC
instead of apparmor in the security stack

I'm not sure what you'd prefer Michal, maybe an early RC=0 exit in
virSecurityMoveRememberedLabel in case of !HAVE_LIBATTR?
Con: That would still fork the process to do nothing then
Pro: It would but be a small change in just one place

Since you did all the related changes I thought I report the case and
leave it up to you Michal, what do you think?


[1]: https://gitlab.com/libvirt/libvirt/-/commit/706e68237f5
[2]: https://gitlab.com/libvirt/libvirt/-/commit/d73f3f58360
[3]: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1879325

> Thanks!
> Michal
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd