mirror of
https://github.com/moby/moby.git
synced 2022-11-09 12:21:53 -05:00
layer: protect from same-name races
As pointed out by Tonis, there's a race between ReleaseRWLayer() and GetRWLayer(): ``` ----- goroutine 1 ----- ----- goroutine 2 ----- ReleaseRWLayer() m := ls.mounts[l.Name()] ... m.deleteReference(l) m.hasReferences() ... GetRWLayer() ... mount := ls.mounts[id] ls.driver.Remove(m.mountID) ls.store.RemoveMount(m.name) return mount.getReference() delete(ls.mounts, m.Name()) ----------------------- ----------------------- ``` When something like this happens, GetRWLayer will return an RWLayer without a storage. Oops. There might be more races like this, and it seems the best solution is to lock by layer id/name by using pkg/locker. With this in place, name collision could not happen, so remove the part of previous commit that protected against it in CreateRWLayer (temporary nil assigmment and associated rollback). So, now we have * layerStore.mountL sync.Mutex to protect layerStore.mount map[] (against concurrent access); * mountedLayer's embedded `sync.Mutex` to protect its references map[]; * layerStore.layerL (which I haven't touched); * per-id locker, to avoid name conflicts and concurrent operations on the same rw layer. The whole rig seems to look more readable now (mutexes use is straightforward, no nested locks). Reported-by: Tonis Tiigi <tonistiigi@gmail.com> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This commit is contained in:
parent
b4e9b50765
commit
af433dd200
1 changed files with 25 additions and 17 deletions
|
@ -10,6 +10,7 @@ import (
|
||||||
"github.com/docker/distribution"
|
"github.com/docker/distribution"
|
||||||
"github.com/docker/docker/daemon/graphdriver"
|
"github.com/docker/docker/daemon/graphdriver"
|
||||||
"github.com/docker/docker/pkg/idtools"
|
"github.com/docker/docker/pkg/idtools"
|
||||||
|
"github.com/docker/docker/pkg/locker"
|
||||||
"github.com/docker/docker/pkg/plugingetter"
|
"github.com/docker/docker/pkg/plugingetter"
|
||||||
"github.com/docker/docker/pkg/stringid"
|
"github.com/docker/docker/pkg/stringid"
|
||||||
"github.com/docker/docker/pkg/system"
|
"github.com/docker/docker/pkg/system"
|
||||||
|
@ -36,6 +37,10 @@ type layerStore struct {
|
||||||
|
|
||||||
mounts map[string]*mountedLayer
|
mounts map[string]*mountedLayer
|
||||||
mountL sync.Mutex
|
mountL sync.Mutex
|
||||||
|
|
||||||
|
// protect *RWLayer() methods from operating on the same name/id
|
||||||
|
locker *locker.Locker
|
||||||
|
|
||||||
os string
|
os string
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -92,6 +97,7 @@ func newStoreFromGraphDriver(root string, driver graphdriver.Driver, os string)
|
||||||
driver: driver,
|
driver: driver,
|
||||||
layerMap: map[ChainID]*roLayer{},
|
layerMap: map[ChainID]*roLayer{},
|
||||||
mounts: map[string]*mountedLayer{},
|
mounts: map[string]*mountedLayer{},
|
||||||
|
locker: locker.New(),
|
||||||
useTarSplit: !caps.ReproducesExactDiffs,
|
useTarSplit: !caps.ReproducesExactDiffs,
|
||||||
os: os,
|
os: os,
|
||||||
}
|
}
|
||||||
|
@ -191,7 +197,7 @@ func (ls *layerStore) loadLayer(layer ChainID) (*roLayer, error) {
|
||||||
func (ls *layerStore) loadMount(mount string) error {
|
func (ls *layerStore) loadMount(mount string) error {
|
||||||
ls.mountL.Lock()
|
ls.mountL.Lock()
|
||||||
defer ls.mountL.Unlock()
|
defer ls.mountL.Unlock()
|
||||||
if m := ls.mounts[mount]; m != nil {
|
if _, ok := ls.mounts[mount]; ok {
|
||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -492,21 +498,15 @@ func (ls *layerStore) CreateRWLayer(name string, parent ChainID, opts *CreateRWL
|
||||||
initFunc = opts.InitFunc
|
initFunc = opts.InitFunc
|
||||||
}
|
}
|
||||||
|
|
||||||
|
ls.locker.Lock(name)
|
||||||
|
defer ls.locker.Unlock(name)
|
||||||
|
|
||||||
ls.mountL.Lock()
|
ls.mountL.Lock()
|
||||||
if _, ok := ls.mounts[name]; ok {
|
_, ok := ls.mounts[name]
|
||||||
ls.mountL.Unlock()
|
ls.mountL.Unlock()
|
||||||
|
if ok {
|
||||||
return nil, ErrMountNameConflict
|
return nil, ErrMountNameConflict
|
||||||
}
|
}
|
||||||
// Avoid name collision by temporary assigning nil
|
|
||||||
ls.mounts[name] = nil
|
|
||||||
ls.mountL.Unlock()
|
|
||||||
defer func() {
|
|
||||||
if err != nil {
|
|
||||||
ls.mountL.Lock()
|
|
||||||
delete(ls.mounts, name)
|
|
||||||
ls.mountL.Unlock()
|
|
||||||
}
|
|
||||||
}()
|
|
||||||
|
|
||||||
var pid string
|
var pid string
|
||||||
var p *roLayer
|
var p *roLayer
|
||||||
|
@ -558,6 +558,9 @@ func (ls *layerStore) CreateRWLayer(name string, parent ChainID, opts *CreateRWL
|
||||||
}
|
}
|
||||||
|
|
||||||
func (ls *layerStore) GetRWLayer(id string) (RWLayer, error) {
|
func (ls *layerStore) GetRWLayer(id string) (RWLayer, error) {
|
||||||
|
ls.locker.Lock(id)
|
||||||
|
defer ls.locker.Unlock(id)
|
||||||
|
|
||||||
ls.mountL.Lock()
|
ls.mountL.Lock()
|
||||||
mount := ls.mounts[id]
|
mount := ls.mounts[id]
|
||||||
ls.mountL.Unlock()
|
ls.mountL.Unlock()
|
||||||
|
@ -570,8 +573,9 @@ func (ls *layerStore) GetRWLayer(id string) (RWLayer, error) {
|
||||||
|
|
||||||
func (ls *layerStore) GetMountID(id string) (string, error) {
|
func (ls *layerStore) GetMountID(id string) (string, error) {
|
||||||
ls.mountL.Lock()
|
ls.mountL.Lock()
|
||||||
defer ls.mountL.Unlock()
|
|
||||||
mount := ls.mounts[id]
|
mount := ls.mounts[id]
|
||||||
|
ls.mountL.Unlock()
|
||||||
|
|
||||||
if mount == nil {
|
if mount == nil {
|
||||||
return "", ErrMountDoesNotExist
|
return "", ErrMountDoesNotExist
|
||||||
}
|
}
|
||||||
|
@ -581,8 +585,12 @@ func (ls *layerStore) GetMountID(id string) (string, error) {
|
||||||
}
|
}
|
||||||
|
|
||||||
func (ls *layerStore) ReleaseRWLayer(l RWLayer) ([]Metadata, error) {
|
func (ls *layerStore) ReleaseRWLayer(l RWLayer) ([]Metadata, error) {
|
||||||
|
name := l.Name()
|
||||||
|
ls.locker.Lock(name)
|
||||||
|
defer ls.locker.Unlock(name)
|
||||||
|
|
||||||
ls.mountL.Lock()
|
ls.mountL.Lock()
|
||||||
m := ls.mounts[l.Name()]
|
m := ls.mounts[name]
|
||||||
ls.mountL.Unlock()
|
ls.mountL.Unlock()
|
||||||
if m == nil {
|
if m == nil {
|
||||||
return []Metadata{}, nil
|
return []Metadata{}, nil
|
||||||
|
@ -617,7 +625,7 @@ func (ls *layerStore) ReleaseRWLayer(l RWLayer) ([]Metadata, error) {
|
||||||
}
|
}
|
||||||
|
|
||||||
ls.mountL.Lock()
|
ls.mountL.Lock()
|
||||||
delete(ls.mounts, m.Name())
|
delete(ls.mounts, name)
|
||||||
ls.mountL.Unlock()
|
ls.mountL.Unlock()
|
||||||
|
|
||||||
ls.layerL.Lock()
|
ls.layerL.Lock()
|
||||||
|
|
Loading…
Add table
Reference in a new issue