Bug 1524792 - RFE: Protect disk metadata with image locking
Summary: RFE: Protect disk metadata with image locking
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Red Hat Enterprise Linux Advanced Virtualization
Classification: Red Hat
Component: libvirt
Version: 8.0
Hardware: x86_64
OS: Unspecified
medium
medium
Target Milestone: rc
: 8.1
Assignee: Michal Privoznik
QA Contact: Han Han
URL:
Whiteboard:
: 1555311 1569500 (view as bug list)
Depends On: 1652078
Blocks: 547546 1584982
TreeView+ depends on / blocked
 
Reported: 2017-12-12 05:50 UTC by Fangge Jin
Modified: 2021-01-08 16:29 UTC (History)
9 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2021-01-08 16:29:30 UTC
Type: Feature Request
Target Upstream Version:


Attachments (Terms of Use)
libvirtd log (125.93 KB, application/zip)
2017-12-12 05:50 UTC, Fangge Jin
no flags Details
lock.c (904 bytes, text/x-csrc)
2019-01-11 10:52 UTC, Michal Privoznik
no flags Details

Description Fangge Jin 2017-12-12 05:50:39 UTC
Created attachment 1366438 [details]
libvirtd log

Description of problem:
Disk image label is restored when another guest using same disk image fails to start

Version-Release number of selected component:
libvirt-3.9.0-5.el7.x86_64

How reproducible:
100%

Steps to Reproduce:
1.Prepare two guests that use same disk image:
    # virsh domblklist rhel7
    Target     Source
    ------------------------------------------------
    vda        /var/lib/libvirt/images/RHEL-7.5-x86_64-latest.qcow2

    # virsh domblklist rhel7-min
    Target     Source
    ------------------------------------------------
    vda        /var/lib/libvirt/images/RHEL-7.5-x86_64-latest.qcow2

2. Start one guest and check disk image label:
    # virsh start rhel7
    Domain rhel7 started

    # ll /var/lib/libvirt/images/RHEL-7.5-x86_64-latest.qcow2 -Z
    -rw-------. qemu qemu system_u:object_r:svirt_image_t:s0:c96,c1016 /var/lib/libvirt/images/RHEL-7.5-x86_64-latest.qcow2

3. Start the other guest and check disk image label:
    # virsh start rhel7-min
    error: Failed to start domain rhel7-min
    error: internal error: process exited while connecting to monitor: 2017-12-10T07:17:30.166423Z qemu-kvm: -drive file=/var/lib/libvirt/images/RHEL-7.5-x86_64-latest.qcow2,format=qcow2,if=none,id=drive-virtio-disk0,cache=none: Failed to get "write" lock
    Is another process using the image?

    # ll /var/lib/libvirt/images/RHEL-7.5-x86_64-latest.qcow2 -Z
    -rw-------. root root system_u:object_r:virt_image_t:s0 /var/lib/libvirt/images/RHEL-7.5-x86_64-latest.qcow2


Actual results:
Disk image label is restored when the second guest fails to start

Expected results:
Disk image label should not be restored when the second guest fails to start, because the first guest will have I/O error.

Comment 2 Michal Privoznik 2017-12-12 12:17:50 UTC
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.

Comment 3 yafu 2018-01-31 06:01:11 UTC
(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.

Comment 4 Michal Privoznik 2018-02-01 10:34:14 UTC
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.

Comment 6 Peter Krempa 2018-03-15 13:56:44 UTC
*** Bug 1555311 has been marked as a duplicate of this bug. ***

Comment 7 Peter Krempa 2018-06-01 06:01:01 UTC
*** Bug 1569500 has been marked as a duplicate of this bug. ***

Comment 9 Michal Privoznik 2018-08-09 13:35:20 UTC
v1 posted into the upstream list:

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

Comment 13 Michal Privoznik 2019-01-11 10:51:49 UTC
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.

Comment 14 Michal Privoznik 2019-01-11 10:52:33 UTC
Created attachment 1520000 [details]
lock.c

Comment 15 Michal Privoznik 2019-03-18 08:17:40 UTC
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.

Comment 16 Michal Privoznik 2019-03-28 13:26:27 UTC
Moving back to ASSIGNED since the referenced bug 547546 is not fully fixed yet. Hence, metadata locking is disabled temporarily for now.

Comment 17 Michal Privoznik 2020-04-02 08:49:53 UTC
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.

Comment 20 Han Han 2020-10-27 03:30:00 UTC
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.

Comment 21 Jeff Nelson 2021-01-08 16:29:30 UTC
RHEL AV 8.3.0 has been shipped, therefore marking this BZ CLOSED CURRENTRELEASE.


Note You need to log in before you can comment on or make changes to this bug.