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

[Peter Krempa]

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 ...

>  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?

> +
> +    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.

> -- 
> 2.26.2
>