From 0f637e09da85b2aaefa279cfb571b004a2cc6d59 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Wed, 11 Nov 2020 15:57:06 -0500 Subject: [PATCH] Ensure we do not double-lock the same volume in create When making containers, we want to lock all named volumes we are adding the container to, to ensure they aren't removed from under us while we are working. Unfortunately, this code did not account for a container having the same volume mounted in multiple places so it could deadlock. Add a map to ensure that we don't lock the same name more than once to resolve this. Fixes #8221 Signed-off-by: Matthew Heon --- libpod/container_internal.go | 1 + libpod/runtime_ctr.go | 7 +++++++ test/e2e/run_volume_test.go | 8 ++++++++ 3 files changed, 16 insertions(+) diff --git a/libpod/container_internal.go b/libpod/container_internal.go index 0aeaae43d1..3a8566760a 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -1503,6 +1503,7 @@ func (c *Container) mountStorage() (_ string, deferredErr error) { // config. // Returns the volume that was mounted. func (c *Container) mountNamedVolume(v *ContainerNamedVolume, mountpoint string) (*Volume, error) { + logrus.Debugf("Going to mount named volume %s", v.Name) vol, err := c.runtime.state.Volume(v.Name) if err != nil { return nil, errors.Wrapf(err, "error retrieving named volume %s for container %s", v.Name, c.ID()) diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go index c842688891..14b537ca2e 100644 --- a/libpod/runtime_ctr.go +++ b/libpod/runtime_ctr.go @@ -345,8 +345,15 @@ func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (_ *Contai // Lock all named volumes we are adding ourself to, to ensure we can't // use a volume being removed. + volsLocked := make(map[string]bool) for _, namedVol := range ctrNamedVolumes { toLock := namedVol + // Ensure that we don't double-lock a named volume that is used + // more than once. + if volsLocked[namedVol.Name()] { + continue + } + volsLocked[namedVol.Name()] = true toLock.lock.Lock() defer toLock.lock.Unlock() } diff --git a/test/e2e/run_volume_test.go b/test/e2e/run_volume_test.go index 1c8a67123b..7c74cea78f 100644 --- a/test/e2e/run_volume_test.go +++ b/test/e2e/run_volume_test.go @@ -540,4 +540,12 @@ VOLUME /test/` session = podmanTest.Podman([]string{"run", "--rm", "-v", fmt.Sprintf("%s:%s:O", mountPath, mountDest), "--mount", fmt.Sprintf("type=tmpfs,target=%s", mountDest), ALPINE}) Expect(session.ExitCode()).To(Not(Equal(0))) }) + + It("same volume in multiple places does not deadlock", func() { + volName := "testVol1" + session := podmanTest.Podman([]string{"run", "-t", "-i", "-v", fmt.Sprintf("%s:/test1", volName), "-v", fmt.Sprintf("%s:/test2", volName), "--rm", ALPINE, "sh", "-c", "mount | grep /test"}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + Expect(len(session.OutputToStringArray())).To(Equal(2)) + }) })