Comment 24 for bug 691590

Revision history for this message
Christian Ehrhardt  (paelzer) wrote : Re: libvirt should not take ownership of ISO images

Interesting indeed.
I remember the patch to restore file ownership, and at least in the cases I looked at it worked fine.
I'm using the normal Ubuntu libvirt in Focal

ii libvirt-daemon 6.0.0-0ubuntu8.1 amd64 Virtualization daemon

That is built without attr support as Soren outlined, a buildlog is here [1]

But with that build it clearly sets and restores file ownership in the common default case:

$ sudo ls -laF /var/lib/libvirt/images/ubuntu18.04.qcow2; virsh start ubuntu18.04; sleep 30s; sudo ls -laF /var/lib/libvirt/images/ubuntu18.04.qcow2; virsh shutdown ubuntu18.04; sleep 30s; sudo ls -laF /var/lib/libvirt/images/ubuntu18.04.qcow2

-rw------- 1 root root 16108814336 Mai 11 14:34 /var/lib/libvirt/images/ubuntu18.04.qcow2
Domain ubuntu18.04 started

-rw------- 1 libvirt-qemu kvm 16108814336 Jun 22 13:02 /var/lib/libvirt/images/ubuntu18.04.qcow2
Domain ubuntu18.04 is being shutdown

-rw------- 1 root root 16108814336 Jun 22 13:03 /var/lib/libvirt/images/ubuntu18.04.qcow2

Just as I remember, file ownership is restored using a build that had --without-attr.

Config for that is in /etc/libvirt/qemu.conf and defaults to
  #dynamic_ownership = 1

Code wise
 This is around virSecurityDACSetOwnership and virSecurityDACRestoreFileLabelInternal.
 I see the xattr store that you mentioned being used in virSecurityGetRememberedLabel
 which would fill the content as you reported.

VIR_SECURITY_MANAGER_DYNAMIC_OWNERSHIP
VIR_FILE_OPEN_FORCE_OWNER

To be clear - I'm not even against enabling attr support it is in main and should be fine, but we have to resolve why it seems to be important for your case but isn't otherwise.

Ok, maybe it generally is only ok because in 99% of the cases the default of root:root was what it began with.
Changing my image and retrying.

$ sudo ls -laF /var/lib/libvirt/images/ubuntu18.04.qcow2; virsh start ubuntu18.04; sleep 30s; sudo ls -laF /var/lib/libvirt/images/ubuntu18.04.qcow2; virsh shutdown ubuntu18.04; sleep 30s; sudo ls -laF /var/lib/libvirt/images/ubuntu18.04.qcow2
-rw------- 1 paelzer root 16108814336 Jun 22 13:27 /var/lib/libvirt/images/ubuntu18.04.qcow2
Domain ubuntu18.04 started

-rw------- 1 libvirt-qemu kvm 16108814336 Jun 22 13:28 /var/lib/libvirt/images/ubuntu18.04.qcow2
Domain ubuntu18.04 is being shutdown

-rw------- 1 root root 16108814336 Jun 22 13:28 /var/lib/libvirt/images/ubuntu18.04.qcow2

Ok, now things make sense.
And as I said libattr1 is in main, the error conditions if the underlying FS has no libattr support are covered by the libvirt code I think. While the impacts as an SRU to existing versions is unclear I agree that we should look into enabling this in >=20.10

[1]: https://launchpad.net/ubuntu/+source/libvirt/6.0.0-0ubuntu8.1/+build/19398123