While I see why this is perceived as a problem I don't think it is actually a bug rather than user error. If an user wants to start two domains which share a disk <shareable/> must be put into both definitions. Failing to do so leads to all sorts of troubles - relabeling is just one of them.

Therefore, I'm closing this one. If you disagree please feel free to reopen stating why you believe this is a bug.

(In reply to Michal Privoznik from comment #2)
> While I see why this is perceived as a problem I don't think it is actually
> a bug rather than user error. If an user wants to start two domains which
> share a disk <shareable/> must be put into both definitions. Failing to do
> so leads to all sorts of troubles - relabeling is just one of them.
> 
> Therefore, I'm closing this one. If you disagree please feel free to reopen
> stating why you believe this is a bug.

Hi, Michal,

When trying to start a guest with images using by other running guest, it is right to fail but maybe not reasonable to restore the image which leads to influence other running guest. If libvirt do that, the qemu lock will be meaningless. And libvirt also restore the label even when setting lock_manager = "lockd" in qemu.conf.
So i reopen the bug, please feel free to close it again if i have any misunderstanding.
Thanks.

Okay, I can see your point. Well, the thing is that currently libvirt protects only disk content with locking and not the metadata (i.e. seclabels). Therefore, libvirt tries to lock the disk before starting vCPUs and releases the lock after vCPUs are stopped. In this case, libvirt starts qemu (which comes with relabelling disk images) and only fails when trying to resume vCPUs (at this point, the disk images are already relabelled).

So I'm turning this into an RFE because while this is certainly not desired behaviour it's a feature.

*** Bug 1555311 has been marked as a duplicate of this bug. ***

*** Bug 1569500 has been marked as a duplicate of this bug. ***

v1 posted into the upstream list:

https://www.redhat.com/archives/libvir-list/2018-August/msg00482.html

v2:

https://www.redhat.com/archives/libvir-list/2018-August/msg00814.html

v3:

https://www.redhat.com/archives/libvir-list/2018-August/msg01627.html

v4:

https://www.redhat.com/archives/libvir-list/2018-September/msg00323.html

Patches for this are in the upstream repo for a while now. However, this feature alone would be hard to test as the locking was a prerequisite for bug 547546. What can be tested though is that whether files that libvirt tries to chown/setfcon are locked. To do that the following steps are needed:

1) make sure that remember_owner is not disabled in qemu.conf (it defaults to enabled)
2) compile the attached file
3) if domain has a disk say /var/lib/libvirt/images/fedora.qcow2 run the following:
   ./lock /var/lib/libvirt/images/fedora.qcow2 1 1
   This locks the second byte in fedora.qcow2 file (which is used by libvirtd)
4) start the domain
5) observe error:
   error: Failed to start domain fedora
   error: internal error: child reported (status=125): unable to lock /var/lib/libvirt/images/fedora.qcow2 for metadata change: Resource temporarily unavailable

This proves that the file is locked when trying to change metadata.

Created attachment 1520000 [details]
lock.c

I suggest making this a TestOnly bug. The point is, we can't lock image metadata for the whole time that an image is in use. Firstly, it would mean the lock has to be distributed across multiple hosts (consider an image stored on an NFS). Secondly, even with the metadata locking we still might want to allow some metadata changes. For instance, assume that one domain has DAC label "user1:group1", and the other domain has "user2:group2". And have user1 be in both group1 and group2. So the first domain starts and the image is relabeled to "user1:group1". If the second domain starts, it will want to chown the imae to "user2:group2" to grant access for the second domain. This should be allowed, because the first domain won't lose its access as user1 is in group2 too. If, however, we locked the metadata and disallowed the change then the second domain won't be allowed to start.

In the light of bug 547546, where (once fixed) libvirt will remember the original owner of the image then we are safe in this bug too. I mean, this bug is a prerequisite for the other. We will need metadata locking as the fix for the referenced bug plays with XATTRs which needs to be atomic. But we will not need the metadata to be locked for the whole use of an image.

Moving back to ASSIGNED since the referenced bug 547546 is not fully fixed yet. Hence, metadata locking is disabled temporarily for now.

We have security label remembering implemented and turned on by default. It prevents two machines accessing the same disk with different labels. Therefore, this is fixed. I'm marking it as TestOnly so that we can verify that metadata are protected.

Verified on libvirt-6.6.0-7.module+el8.3.0+8424+5ea525c5.x86_64 qemu-kvm-5.1.0-13.module+el8.3.0+8382+afc3bbea.x86_64:
1. Start an VM
➜  virsh start new
Domain new started

2. Get the metadata of the VM disk file:
➜  ~ getfattr -m trusted.libvirt.security -d /var/lib/libvirt/images/new.qcow2 
getfattr: Removing leading '/' from absolute path names
# file: var/lib/libvirt/images/new.qcow2
trusted.libvirt.security.dac="+107:+107"
trusted.libvirt.security.ref_dac="1"
trusted.libvirt.security.ref_selinux="1"
trusted.libvirt.security.selinux="system_u:object_r:virt_content_t:s0"
trusted.libvirt.security.timestamp_dac="1603666746"
trusted.libvirt.security.timestamp_selinux="1603666746"

➜  ~                                                                          

➜  ~ ll /var/lib/libvirt/images/new.qcow2
-rw-r--r--. 1 qemu qemu 1.5G Oct 26 23:25 /var/lib/libvirt/images/new.qcow2


3. Try to start an VM with the same disk file path. Then check the metadata again:
➜  ~ virsh create /tmp/new1.xml          
error: Failed to create domain from /tmp/new1.xml
error: Requested operation is not valid: Setting different SELinux label on /var/lib/libvirt/images/new.qcow2 which is already in use

➜  ~ ll /var/lib/libvirt/images/new.qcow2
-rw-r--r--. 1 qemu qemu 1.5G Oct 26 23:25 /var/lib/libvirt/images/new.qcow2
➜  ~ getfattr -m trusted.libvirt.security -d /var/lib/libvirt/images/new.qcow2
getfattr: Removing leading '/' from absolute path names
# file: var/lib/libvirt/images/new.qcow2
trusted.libvirt.security.dac="+107:+107"
trusted.libvirt.security.ref_dac="1"
trusted.libvirt.security.ref_selinux="1"
trusted.libvirt.security.selinux="system_u:object_r:virt_content_t:s0"
trusted.libvirt.security.timestamp_dac="1603666746"
trusted.libvirt.security.timestamp_selinux="1603666746"

The metadata is unchanged.

RHEL AV 8.3.0 has been shipped, therefore marking this BZ CLOSED CURRENTRELEASE.