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

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(a)redhat.com&gt; > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > --- > 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.

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.