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

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(a)redhat.com&gt; > --- > 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.