Skip to content

Commit

Permalink
podman volume rm --force: fix ABBA deadlock
Browse files Browse the repository at this point in the history
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 3cc9db8 due another deadlock there.

Fixes #23613

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
  • Loading branch information
Luap99 committed Aug 15, 2024
1 parent b6beed9 commit a65aecd
Showing 1 changed file with 14 additions and 7 deletions.
21 changes: 14 additions & 7 deletions libpod/runtime_volume_common.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down

0 comments on commit a65aecd

Please sign in to comment.