Created attachment 1678836 [details]
Test script for dm_set_task_name()

Description of problem:

This problem is easiest to see by creating a KVM virtual machine that uses a disk referencing a block device that is not a device mapper device but where a device mapper device with the same name also exists. The easiest example is to do this with the device /dev/loop0 and another device /dev/mapper/loop0. Note that /dev/loop0 and /dev/mapper/loop0 have nothing to do with each other, but they share the same filename (but not path!).

When libvirt starts a virtual machine, it reaches virDevMapperGetTargetsImpl() in virdevmapper.c which attempts to call dm_task_set_name() on /dev/loop0.

dm_task_set_name() realizes that this is a path, and calls _dm_task_set_name_from_path() which attempts to strip the name from the path ("loop0") and assumes that this could be the same as /dev/mapper/loop0. However, it is smart enough to check that st_rdev is the same for both /dev/loop0 and /dev/mapper/loop0. Since they are different (of course), it then invokes _find_dm_name_of_device() to scan the /dev/mapper directory for an entry that has the same st_rdev value, which of course it will not find. Since it cannot be found, it returns the rather confusing error from device mapper:
    "Device loop0 not found"

And since errno is set to 0 ("Success", not ENOENT), virDevMapperGetTargetsImpl() then gives the confusing error:
    "error: Unable to get devmapper targets for /dev/loop0: Success"

This is only an issue because there exists a filename /dev/mapper/loop0. If we remove /dev/mapper/loop0, but keep the virtual machine booting from /dev/loop0, we still get the same error "Device loop0 not found" from device mapper, but this time errno is set to ENOENT so virDevMapperGetTargetsImpl() ignores the failure and boots the VM because of this branch in the function:
    if (!dm_task_set_name(dmt, path)) {
        if (errno == ENOENT) {
            /* It's okay, @path is not managed by devmapper =>
             * not a devmapper device. */
            ret = 0;
        }
        goto cleanup;
    }

Note that the code above only assumes that "@path is not managed by devmapper" because dm_task_set_name() is expected to set errno == ENOENT, but it does not.

It is unclear whether this bug lies in libvirt or libdevmapper. Perhaps libvirt should not invoke libdevmapper calls on non-device mapper paths (check major = 253?)---or perhaps libdevmapper should fail early if the referenced path is a block device and does not have a major of 253. In the latter case, ENOENT is probably nonsensical because /dev/loop0 does exist but is not a device mapper target, so perhaps EINVALID is a better errno value in this case. At the moment I believe errno is being set by libc calls and not by libdevmapper directly so it may or may not be appropriate for libdevmapper to set errno. If errno were set to EINVALID by libdevmapper, then libvirt would have to support that as an "okay" reason to set ret = 0 and goto cleanup in virDevMapperGetTargetsImpl().

In any event, it may be necessary for both libdevmapper and libvirt teams to decide what the best solution is here, both packages may need to be modified to solve the problem.

Version-Release number of selected component (if applicable):

device-mapper-1.02.158-2.el7_7.2.x86_64
lvm2-2.02.185-2.el7_7.2.x86_64
libvirt-4.5.0-23.el7_7.6.x86_64


How reproducible: very


Steps to Reproduce:
1. Create a device mapper target called "loop0":
~]# dmsetup create --table '0 10240 zero' loop0

2. Create a loopback device to boot a virtual machine:
~]# losetup /dev/loop0 /some/bootimg.raw

3. virsh start someVmName:
~]# virsh start someVmName
error: Failed to start domain someVmName
error: Unable to get devmapper targets for /dev/loop0: Success

4. Remove the DM loop0 device and try again:
~]# dmsetup remove loop0
~]# virsh start someVmName
Domain someVmName started


Actual results:

VM fails to start with error:
error: Failed to start domain someVmName
error: Unable to get devmapper targets for /dev/loop0: Success


Expected results:

VM should start


Additional info:

In order to test the device mapper segment of this problem we wrote a small .c program (attached) which you can invoke as follows:
# To build:
~]# gcc -ldevmapper test-dm_set_task_name.c -o test-dm_set_task_name

# To test the case where libvirt would not give an error because it detects ENOENT:
~]# ./test-dm_set_task_name /dev/loop0
dm version   [ opencount flush ]   [16384] (*1)
Device loop0 not found
errno: No such file or directory

# To test the case where libvirt would give an error because errno = "Success"
~]# dmsetup create --table '0 10240 zero' loop0
~]# ./test-dm_set_task_name /dev/loop0 
dm version   [ opencount flush ]   [16384] (*1)
Device loop0 not found
errno: Success