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@xxxxxxxxxx>
Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
---
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
