[PATCH 1/2] virdevmapper: Don't error on kernels without DM support

Michal Privoznik mprivozn at redhat.com
Mon Aug 17 15:40:04 UTC 2020


On 8/17/20 5:16 PM, Peter Krempa wrote:
> On Mon, Aug 17, 2020 at 16:26:54 +0200, Michal Privoznik wrote:
>> In one of my latest patch (v6.6.0~30) I was trying to remove
>> libdevmapper use in favor of our own implementation. However, the
>> code did not take into account that device mapper can be not
>> compiled into the kernel (e.g. be a separate module that's not
>> loaded) in which case /proc/devices won't have the device-mapper
>> major number and thus virDevMapperGetTargets() and/or
>> virIsDevMapperDevice() fails.
>>
>> Fixes: 22494556542c676d1b9e7f1c1f2ea13ac17e1e3e
>> Reported-by: Andrea Bolognani <abologna at redhat.com>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>   src/util/virdevmapper.c | 41 +++++++++++++++++++++++++++++++----------
>>   1 file changed, 31 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c
>> index fe7f611496..cc33d8211e 100644
>> --- a/src/util/virdevmapper.c
>> +++ b/src/util/virdevmapper.c
>> @@ -47,10 +47,11 @@
>>   G_STATIC_ASSERT(BUF_SIZE > sizeof(struct dm_ioctl));
>>   
>>   static unsigned int virDMMajor;
>> +static virMutex virDevMapperInitMutex = VIR_MUTEX_INITIALIZER;
> 
> 'virDMMajor' should now be initialized explicitly ...

Aren't global static variables initialized to zero automatically 
(something about .bss section)?

> 
>>   static int
>> -virDevMapperOnceInit(void)
>> +virDevMapperGetMajor(unsigned int *dmmaj)
>>   {
>>       g_autofree char *buf = NULL;
>>       VIR_AUTOSTRINGLIST lines = NULL;
>> @@ -69,23 +70,34 @@ virDevMapperOnceInit(void)
>>   
>>           if (sscanf(lines[i], "%u %ms\n", &maj, &dev) == 2 &&
>>               STREQ(dev, DM_NAME)) {
>> -            virDMMajor = maj;
> 
> ... since it's not always initialized here.
> 
>> +            *dmmaj = maj;
>>               break;
>>           }
>>       }
>>   
>>       if (!lines[i]) {
>> -        virReportError(VIR_ERR_INTERNAL_ERROR,
>> -                       _("Unable to find major for %s"),
>> -                       DM_NAME);
>> -        return -1;
>> +        /* Don't error here. Maybe the kernel was built without
>> +         * devmapper. Let the caller decide. */
>> +        return -2;
>>       }
>>   
>>       return 0;
>>   }
>>   
>>   
>> -VIR_ONCE_GLOBAL_INIT(virDevMapper);
>> +static int
>> +virDevMapperInit(void)
>> +{
>> +    int rc = 0;
>> +
>> +    virMutexLock(&virDevMapperInitMutex);
>> +
>> +    if (virDMMajor == 0)
> 
> I'm not quite persuaded with this caching here. For cases when the
> device mapper is loaded we cache it, but in the negative case ...
> 
>> +        rc = virDevMapperGetMajor(&virDMMajor);
> 
> ... we always process /proc/devices. Why is caching necessary then?

Are you suggesting to parse /proc/devices every time? My concern is it 
will burn CPU cycles needlessly. And I'm not sure how to address the 
negative case when DM module is not loaded. It's ineffective, I agree, I 
just don't see how to make it better.

> 
>> +
>> +    virMutexUnlock(&virDevMapperInitMutex);
>> +    return rc;
>> +}
>>   
>>   
>>   static void *
>> @@ -220,7 +232,6 @@ virDevMapperGetTargetsImpl(int controlFD,
>>       size_t i;
>>   
>>       memset(&dm, 0, sizeof(dm));
>> -    *devPaths_ret = NULL;
>>   
>>       if (ttl == 0) {
>>           errno = ELOOP;
>> @@ -298,14 +309,24 @@ virDevMapperGetTargets(const char *path,
>>                          char ***devPaths)
>>   {
>>       VIR_AUTOCLOSE controlFD = -1;
>> +    int rc;
>>       const unsigned int ttl = 32;
>>   
>>       /* Arbitrary limit on recursion level. A devmapper target can
>>        * consist of devices or yet another targets. If that's the
>>        * case, we have to stop recursion somewhere. */
>>   
>> -    if (virDevMapperInitialize() < 0)
>> +    *devPaths = NULL;
>> +
>> +    if ((rc = virDevMapperInit()) < 0) {
>> +        if (rc == -2) {
>> +            /* Devmapper is not available. Yet. Maybe the module
>> +             * will be loaded later, but do not error out for now. */
>> +            return 0;
>> +        }
>> +
>>           return -1;
>> +    }
>>   
>>       if ((controlFD = virDMOpen()) < 0)
>>           return -1;
>> @@ -319,7 +340,7 @@ virIsDevMapperDevice(const char *dev_name)
>>   {
>>       struct stat buf;
>>   
>> -    if (virDevMapperInitialize() < 0)
>> +    if (virDevMapperInit() < 0)
>>           return false;
>>   
>>       if (!stat(dev_name, &buf) &&
> 
> Code after this hunk reads 'virDMMajor' directly without locks. Since
> virDevMapperInit returns it here it should be used from the return value
> rather than accessed directly which creates a code-pattern open for race
> conditions.

I'm not sure I follow. virDevMapperInit() returns -2, -1, or 0. Not the 
major. And reading without lock is fine because there is no way that the 
virDMMajor is not set at this point (in which case the 
virDevMapperInit() is basically a NOP. But if it makes us feel better I 
can put some locking around.

Michal




More information about the libvir-list mailing list