From a65aecd2604addd14290971f09e894d408ff86d2 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Wed, 14 Aug 2024 11:55:16 +0200 Subject: [PATCH] podman volume rm --force: fix ABBA deadlock We cannot get first the volume lock and the container locks. Other code paths always have to first lock the container and the lock the volumes, i.e. to mount/umount them. As such locking the volume fust can always result in ABBA deadlocks. To fix this move the lock down after the container removal. The removal code is racy regardless of the lock as the volume lcok on create is no longer taken since commit 3cc9db8626 due another deadlock there. Fixes #23613 Signed-off-by: Paul Holzinger --- libpod/runtime_volume_common.go | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/libpod/runtime_volume_common.go b/libpod/runtime_volume_common.go index 44d4f667d7..532ea195a3 100644 --- a/libpod/runtime_volume_common.go +++ b/libpod/runtime_volume_common.go @@ -357,13 +357,11 @@ func (r *Runtime) removeVolume(ctx context.Context, v *Volume, force bool, timeo return define.ErrVolumeRemoved } - v.lock.Lock() - defer v.lock.Unlock() - - // Update volume status to pick up a potential removal from state - if err := v.update(); err != nil { - return err - } + // DANGEROUS: Do not lock here yet because we might needed to remove containers first. + // In general we must always acquire the ctr lock before a volume lock so we cannot lock. + // THIS MUST BE DONE to prevent ABBA deadlocks. + // It also means the are several races around creating containers with volumes and removing + // them in parallel. However that problem exists regadless of taking the lock here or not. deps, err := r.state.VolumeInUse(v) if err != nil { @@ -400,6 +398,15 @@ func (r *Runtime) removeVolume(ctx context.Context, v *Volume, force bool, timeo } } + // Ok now we are good to lock. + v.lock.Lock() + defer v.lock.Unlock() + + // Update volume status to pick up a potential removal from state + if err := v.update(); err != nil { + return err + } + // If the volume is still mounted - force unmount it if err := v.unmount(true); err != nil { if force {