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