[PATCH 2/2] virdevmapper: Deal with unloading dm module

[Michal Privoznik]

On 8/17/20 5:28 PM, Peter Krempa wrote:
> On Mon, Aug 17, 2020 at 16:26:55 +0200, Michal Privoznik wrote:
>> The following situation can happen: we've initialized DM major
>> successfully. But later, the devmapper module(s) was removed and
>> thus kernel lost its ability to handle multipath devices. More
>> importantly, the /dev/mapper/control file is removed and thus our
>> attempt to open it will fail. Note, we will attempt to open it
>> only after we've found devmapper major number successfully in one
>> of the previous runs. Anyway, if it so happens that the module
>> was unloaded then we have to reset the major number we remembered
>> from one of the previous runs and not report error.
>>
>> Fixes: 22494556542c676d1b9e7f1c1f2ea13ac17e1e3e
>> Reported-by: Andrea Bolognani <abologna at redhat.com>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>   src/util/virdevmapper.c | 18 ++++++++++++++++--
>>   1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c
>> index cc33d8211e..642f1d9bdb 100644
>> --- a/src/util/virdevmapper.c
>> +++ b/src/util/virdevmapper.c
>> @@ -143,8 +143,13 @@ virDMOpen(void)
>>   
>>       memset(&dm, 0, sizeof(dm));
>>   
>> -    if ((controlFD = open(CONTROL_PATH, O_RDWR)) < 0)
>> +    if ((controlFD = open(CONTROL_PATH, O_RDWR)) < 0) {
>> +        if (errno == ENOENT)
>> +            return -2;
>> +
>> +        virReportSystemError(errno, _("Unable to open %s"), CONTROL_PATH);
>>           return -1;
>> +    }
>>   
>>       if (!virDMIoctl(controlFD, DM_VERSION, &dm, &tmp)) {
>>           virReportSystemError(errno, "%s",
>> @@ -328,8 +333,17 @@ virDevMapperGetTargets(const char *path,
>>           return -1;
>>       }
>>   
>> -    if ((controlFD = virDMOpen()) < 0)
>> +    if ((controlFD = virDMOpen()) < 0) {
>> +        if (controlFD == -2) {
>> +            /* Devmapper was available but now it isn't. Somebody
>> +             * must have removed the module. Reset the major
>> +             * number we remember. */
>> +            virDMMajor = 0;
> 
> This overwrites 'virDMMajor' without taking the mutex. 'unsigned int' on
> x86_64 will be atomic, we shouldn't use a knowingly broken code pattern
> without at least a proper explanation why it's okay.
> 
> Also as noted in review of previous patch this looks shady and
> error-prone. If the unloading and re-loading of the device mapper module
> changes the 'major' number of number we use internally for checking,
> then caching it without validating that somebody didn't unload and
> reload the module between two calls to virDMOpen is wrong as we might
> use the wrong number.

I haven't looked into the kernel code, but IIRC even libdevmapper 
considered the major number immutable. Let me dive into the code.

> 
> In case when for a given kernel it can only have one value regardless of
> how you load the module, you can cache it without the mutex dance after
> calling virDMOpen and remember it for ever, you'll still be gated
> whether the control device path exists.

The dance with locks is to have only one thread parse /proc/devices 
instead of all of them at once. That's why I did not bother guarding 
other accesses to virDMMajor with locks.

Michal