[PATCH v1 01/34] virDevMapperGetTargetsImpl: Close /dev/mapper/control in the end

[Daniel P. Berrangé]

On Wed, Jul 22, 2020 at 11:39:55AM +0200, Michal Privoznik wrote:
> When building domain's private /dev in a namespace, libdevmapper
> is consulted for getting full dependency tree of domain's disks.
> The reason is that for a multipath devices all dependent devices
> must be created in the namespace and allowed in CGroups.
> 
> However, this approach is very fragile as building of namespace
> happens in the forked off child process, after mass close of FDs
> and just before dropping privileges and execing QEMU. And it so
> happens that when calling libdevmapper APIs, one of them opens
> /dev/mapper/control and saves the FD into a global variable. The
> FD is kept open until the lib is unlinked or dm_lib_release() is
> called explicitly. We are doing neither.
> 
> This is not a problem when calling the function from libvirtd
> (when setting up CGroups), but it is a problem when called from
> the pre-exec hook because we leak the FD into QEMU.
> 
> Fixes: a30078cb832646177defd256e77c632905f1e6d0
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1858260
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/util/virdevmapper.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c
> index 40a82285f9..1c216fb6c1 100644
> --- a/src/util/virdevmapper.c
> +++ b/src/util/virdevmapper.c
> @@ -156,6 +156,7 @@ virDevMapperGetTargetsImpl(const char *path,
>      virStringListFree(recursiveDevPaths);
>      virStringListFree(devPaths);
>      dm_task_destroy(dmt);
> +    dm_lib_release();
>      return ret;
>  }

Hmm, this function isn't threadsafe though, so I'm kind of worried about
us breaking parallel callers.

For that matter dm_task_run isn't thread safe when it opens the
FD in the first place either AFAICT.

This libdevice-mapper.so looks like a bit of a disaster in general, as
I can't see mutexes acquired anywhere in the public APIs and it has a
load of static global variables including this FD :-(

We could put mutex locking around our own virDevMapperGetTargetsImpl
if we blindly assume nothing else we call may secretly be using
libdevice-mapper too.

Makes me wonder though if there's any way we can obtain the info we need
without touching libdevice-mapper.so at all.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|