[PATCH v1 01/34] virDevMapperGetTargetsImpl: Close /dev/mapper/control in the end
Daniel P. Berrangé
berrange at redhat.com
Wed Jul 22 10:46:44 UTC 2020
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 :|
More information about the libvir-list
mailing list