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

On 5/25/20 1:14 PM, Christian Ehrhardt wrote: > On Mon, May 18, 2020 at 10:07 AM Michal Privoznik <mprivozn(a)redhat.com&gt; 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 Thank you for your deep analysis. > > 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? Yes, a naive fix would be something like this: diff --git i/src/security/security_dac.c w/src/security/security_dac.c index bdc2d7edf3..7b95a6f86d 100644 --- i/src/security/security_dac.c +++ w/src/security/security_dac.c @@ -1117,6 +1117,12 @@ virSecurityDACMoveImageMetadataHelper(pid_t pid G_GNUC_UNUSED, ret = virSecurityMoveRememberedLabel(SECURITY_DAC_NAME, data->src, data->dst); virSecurityManagerMetadataUnlock(data->mgr, &state); + + if (ret == -2) { + /* Libvirt built without XATTRS */ + ret = 0; + } + return ret; } But as you correctly say, it will still lead to an unnecessary spawn of a process that will do NOP (basically). I think a better fix would be to not require .domainMoveImageMetadata callbacks (i.e. the one from NOP driver could be then removed) and set the DAC/SELinux callbacks if and only if HAVE_LIBATTR; alternatively, make those functions be NOP if !HAVE_LIBATTR. On the other hand, every time we relabel a path outside of /dev, we technically spawn unnecessary because only /dev lives inside a private NS. Everything else is from the parent NS. For instance, there is no need to spawn only to chown("/var/lib/libvirt/images/fedora.qcow2"). Therefore I might have a slight preference for the naive fix, but I will leave it up to you. Or do you want me to sent the patch?

Michal