From 6c173a412b9ec5df52a8a24c5df2e8a52ef52772 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 21 May 2024 21:24:38 +0200 Subject: [PATCH 1/4] Use the right unlock function on error paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miloslav Trmač --- pkg/lockfile/lockfile.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/lockfile/lockfile.go b/pkg/lockfile/lockfile.go index 5101475786..faf995a2d6 100644 --- a/pkg/lockfile/lockfile.go +++ b/pkg/lockfile/lockfile.go @@ -426,10 +426,13 @@ func (l *LockFile) lock(lType lockType) { // command. func (l *LockFile) tryLock(lType lockType) error { var success bool + var rwMutexUnlocker func() if lType == readLock { success = l.rwMutex.TryRLock() + rwMutexUnlocker = l.rwMutex.RUnlock } else { success = l.rwMutex.TryLock() + rwMutexUnlocker = l.rwMutex.Unlock } if !success { return fmt.Errorf("resource temporarily unavailable") @@ -440,7 +443,7 @@ func (l *LockFile) tryLock(lType lockType) error { // If we're the first reference on the lock, we need to open the file again. fd, err := openLock(l.file, l.ro) if err != nil { - l.rwMutex.Unlock() + rwMutexUnlocker() return err } l.fd = fd @@ -450,7 +453,7 @@ func (l *LockFile) tryLock(lType lockType) error { // reader lock or a writer lock. if err = lockHandle(l.fd, lType, true); err != nil { closeHandle(fd) - l.rwMutex.Unlock() + rwMutexUnlocker() return err } } From 5bf717bb16518833d5a96dd716bf0f9709b05ec0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 21 May 2024 21:39:13 +0200 Subject: [PATCH 2/4] Add a comment to help future maintainers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Should not change behavior. Signed-off-by: Miloslav Trmač --- store.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/store.go b/store.go index 957675ba46..888b3c8635 100644 --- a/store.go +++ b/store.go @@ -3009,6 +3009,9 @@ func (s *store) ApplyStagedLayer(args ApplyStagedLayerOptions) (*Layer, error) { return layer, err } if err == nil { + // This code path exists only for cmd/containers/storage.applyDiffUsingStagingDirectory; we have tests that + // assume layer creation and applying a staged layer are separate steps. Production pull code always uses the + // other path, where layer creation is atomic. return layer, rlstore.applyDiffFromStagingDirectory(args.ID, args.DiffOutput, args.DiffOptions) } From 7ca2113a63f47d9a692af6ea008a0cafdaf54907 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 21 May 2024 21:35:38 +0200 Subject: [PATCH 3/4] Move the containerStore locking inside putLayer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There's no need for the callers to deal with this, and now the API of the function is much less surprising. Also fix the documentation about locking rlstores. Signed-off-by: Miloslav Trmač --- store.go | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/store.go b/store.go index 888b3c8635..05d9c948cb 100644 --- a/store.go +++ b/store.go @@ -1437,8 +1437,15 @@ func (s *store) canUseShifting(uidmap, gidmap []idtools.IDMap) bool { return true } -// putLayer requires the rlstore, rlstores, as well as s.containerStore (even if not an argument to this function) to be locked for write. +// On entry: +// - rlstore must be locked for writing +// - rlstores MUST NOT be locked func (s *store) putLayer(rlstore rwLayerStore, rlstores []roLayerStore, id, parent string, names []string, mountLabel string, writeable bool, lOptions *LayerOptions, diff io.Reader, slo *stagedLayerOptions) (*Layer, int64, error) { + if err := s.containerStore.startWriting(); err != nil { + return nil, -1, err + } + defer s.containerStore.stopWriting() + var parentLayer *Layer var options LayerOptions if lOptions != nil { @@ -1525,10 +1532,6 @@ func (s *store) PutLayer(id, parent string, names []string, mountLabel string, w return nil, -1, err } defer rlstore.stopWriting() - if err := s.containerStore.startWriting(); err != nil { - return nil, -1, err - } - defer s.containerStore.stopWriting() return s.putLayer(rlstore, rlstores, id, parent, names, mountLabel, writeable, lOptions, diff, nil) } @@ -3017,11 +3020,6 @@ func (s *store) ApplyStagedLayer(args ApplyStagedLayerOptions) (*Layer, error) { // if the layer doesn't exist yet, try to create it. - if err := s.containerStore.startWriting(); err != nil { - return nil, err - } - defer s.containerStore.stopWriting() - slo := stagedLayerOptions{ DiffOutput: args.DiffOutput, DiffOptions: args.DiffOptions, From 830c7c6383c6fce5a76ec3799efde502a4e3e102 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 21 May 2024 21:46:18 +0200 Subject: [PATCH 4/4] Fix lock hierarchy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As documented at the top of the store type, the roLayerStores must be locked _before_ the container store. This is a minimal / conservative fix; this probably could use a read-only lock only for the duration of the "LayerID == parent" check. Signed-off-by: Miloslav Trmač --- store.go | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/store.go b/store.go index 05d9c948cb..181086c6a5 100644 --- a/store.go +++ b/store.go @@ -1441,11 +1441,6 @@ func (s *store) canUseShifting(uidmap, gidmap []idtools.IDMap) bool { // - rlstore must be locked for writing // - rlstores MUST NOT be locked func (s *store) putLayer(rlstore rwLayerStore, rlstores []roLayerStore, id, parent string, names []string, mountLabel string, writeable bool, lOptions *LayerOptions, diff io.Reader, slo *stagedLayerOptions) (*Layer, int64, error) { - if err := s.containerStore.startWriting(); err != nil { - return nil, -1, err - } - defer s.containerStore.stopWriting() - var parentLayer *Layer var options LayerOptions if lOptions != nil { @@ -1481,6 +1476,11 @@ func (s *store) putLayer(rlstore rwLayerStore, rlstores []roLayerStore, id, pare return nil, -1, ErrLayerUnknown } parentLayer = ilayer + + if err := s.containerStore.startWriting(); err != nil { + return nil, -1, err + } + defer s.containerStore.stopWriting() containers, err := s.containerStore.Containers() if err != nil { return nil, -1, err @@ -1497,6 +1497,13 @@ func (s *store) putLayer(rlstore rwLayerStore, rlstores []roLayerStore, id, pare gidMap = ilayer.GIDMap } } else { + // FIXME? It’s unclear why we are holding containerStore locked here at all + // (and because we are not modifying it, why it is a write lock, not a read lock). + if err := s.containerStore.startWriting(); err != nil { + return nil, -1, err + } + defer s.containerStore.stopWriting() + if !options.HostUIDMapping && len(options.UIDMap) == 0 { uidMap = s.uidMap }