From 9ef352adfc73821465515d746723930c9b98670e Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Wed, 11 Oct 2023 11:21:56 +0200 Subject: [PATCH] [v4.4.1-rhel] libpod: fix deadlock while parallel container create Cherry pick from #20329 Addresses: https://issues.redhat.com/browse/RHEL-14744 and https://issues.redhat.com/browse/RHEL-14743 When containers are created with a named volume it can deadlock because the create logic tried to lock all volumes in a loop, this is fine if it only ever creates a single container at any given time. However because we multiple containers can be created at the same time they can cause a deadlock between the volumes. This is because the order of the loop is not stable, in fact it is based on the order of how the volumes were specified on the cli. So if you create two containers at the same time with `-v vol1:/dir2 -v vol2:/dir2` and the other one with `-v vol2:/dir2 -v vol1:/dir1` then there is chance for a deadlock. Now one solution could be to order the volumes to prevent the issue but the reason for holding the lock is dubious. The goal was to prevent the volume from being removed in the meantime. However that could still have happend before we acquired the lock so it didn't protect against that. Both boltdb and sqlite already prevent us from adding a container with volumes that do not exists due their internal consistency checks. Sqlite even uses FOREIGN KEY relationships so the schema will prevent us from doing anything wrong. The create code currently first checks if the volume exists and if not creates it. I have checked that the db will guarantee that this will not work: Boltdb: `no volume with name test2 found in database when adding container xxx: no such volume` Sqlite: `adding container volume test2 to database: FOREIGN KEY constraint failed` Keep in mind that this error is normally not seen, only if the volume is removed between the volume exists check and adding the container in the db this messages will be seen wich is an acceptable race and a pre-existing condition anyway. [NO NEW TESTS NEEDED] Race condition, hard to test in CI. Fixes #20313 Signed-off-by: Paul Holzinger Signed-off-by: tomsweeneyredhat --- libpod/runtime_ctr.go | 25 +++---------------------- 1 file changed, 3 insertions(+), 22 deletions(-) diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go index 33e31c63e3..52216e19dc 100644 --- a/libpod/runtime_ctr.go +++ b/libpod/runtime_ctr.go @@ -467,8 +467,6 @@ func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (_ *Contai // Go through named volumes and add them. // If they don't exist they will be created using basic options. - // Maintain an array of them - we need to lock them later. - ctrNamedVolumes := make([]*Volume, 0, len(ctr.config.NamedVolumes)) for _, vol := range ctr.config.NamedVolumes { isAnonymous := false if vol.Name == "" { @@ -477,10 +475,9 @@ func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (_ *Contai vol.Name = stringid.GenerateRandomID() isAnonymous = true } else { - // Check if it exists already - dbVol, err := r.state.Volume(vol.Name) + // Check if it already exists + _, err := r.state.Volume(vol.Name) if err == nil { - ctrNamedVolumes = append(ctrNamedVolumes, dbVol) // The volume exists, we're good continue } else if !errors.Is(err, define.ErrNoSuchVolume) { @@ -532,12 +529,10 @@ func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (_ *Contai volOptions = append(volOptions, WithVolumeNoChown()) } - newVol, err := r.newVolume(ctx, false, volOptions...) + _, err = r.newVolume(ctx, false, volOptions...) if err != nil { return nil, fmt.Errorf("creating named volume %q: %w", vol.Name, err) } - - ctrNamedVolumes = append(ctrNamedVolumes, newVol) } switch ctr.config.LogDriver { @@ -563,20 +558,6 @@ func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (_ *Contai ctr.config.Mounts = append(ctr.config.Mounts, ctr.config.ShmDir) } - // 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() - } // Add the container to the state // TODO: May be worth looking into recovering from name/ID collisions here if ctr.config.Pod != "" {