On 5/25/20 1:14 PM, Christian Ehrhardt wrote:
On Mon, May 18, 2020 at 10:07 AM Michal Privoznik <mprivozn@xxxxxxxxxx> 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
