From afa4fef2c84e9a70f672f948690262999a46c932 Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Fri, 16 Aug 2024 10:34:52 -0700 Subject: [PATCH] Add +checklocks:mu to Loader. This change adds +checklocks:mu to boot.Loader. This will help to ensure the loader's mutex is held when accessing its protected fields. This change also fixes some missing locks in controller.go and restore.go. These locks are needed to ensure that these fields are accessed in a thread-safe manner. PiperOrigin-RevId: 663776916 --- runsc/boot/controller.go | 16 ++++++++++++++-- runsc/boot/loader.go | 25 ++++++++++++++++--------- runsc/boot/loader_test.go | 2 ++ runsc/boot/restore.go | 12 ++++++------ 4 files changed, 38 insertions(+), 17 deletions(-) diff --git a/runsc/boot/controller.go b/runsc/boot/controller.go index 09146ce7d..b76336fb6 100644 --- a/runsc/boot/controller.go +++ b/runsc/boot/controller.go @@ -488,6 +488,10 @@ type RestoreOpts struct { func (cm *containerManager) Restore(o *RestoreOpts, _ *struct{}) error { log.Debugf("containerManager.Restore") + cm.l.mu.Lock() + cu := cleanup.Make(cm.l.mu.Unlock) + defer cu.Clean() + if cm.l.state == restoring { return fmt.Errorf("restore is already in progress") } @@ -514,6 +518,8 @@ func (cm *containerManager) Restore(o *RestoreOpts, _ *struct{}) error { cm.restorer = &restorer{restoreDone: cm.onRestoreDone, stateFile: stateFile} cm.l.restoreWaiters = sync.NewCond(&cm.l.mu) cm.l.state = restoring + // Release `cm.l.mu`. + cu.Clean() fileIdx := 1 if o.HavePagesFile { @@ -590,9 +596,12 @@ func (cm *containerManager) onRestoreDone() error { func (cm *containerManager) RestoreSubcontainer(args *StartArgs, _ *struct{}) error { log.Debugf("containerManager.RestoreSubcontainer, cid: %s, args: %+v", args.CID, args) + cm.l.mu.Lock() if cm.l.state != restoring { + cm.l.mu.Unlock() return fmt.Errorf("sandbox is not being restored, cannot restore subcontainer") } + cm.l.mu.Unlock() // Validate arguments. if args.Spec == nil { @@ -808,8 +817,9 @@ func (cm *containerManager) ProcfsDump(_ *struct{}, out *[]procfs.ProcessProcfsD log.Debugf("containerManager.ProcfsDump") ts := cm.l.k.TaskSet() pidns := ts.Root - *out = make([]procfs.ProcessProcfsDump, 0, len(cm.l.processes)) - for _, tg := range pidns.ThreadGroups() { + tgs := pidns.ThreadGroups() + *out = make([]procfs.ProcessProcfsDump, 0, len(tgs)) + for _, tg := range tgs { pid := pidns.IDOfThreadGroup(tg) procDump, err := procfs.Dump(tg.Leader(), pid, pidns) if err != nil { @@ -848,6 +858,8 @@ func (cm *containerManager) Mount(args *MountArgs, _ *struct{}) error { var cu cleanup.Cleanup defer cu.Clean() + cm.l.mu.Lock() + defer cm.l.mu.Unlock() eid := execID{cid: args.ContainerID} ep, ok := cm.l.processes[eid] if !ok { diff --git a/runsc/boot/loader.go b/runsc/boot/loader.go index 1c04de808..94f925b8a 100644 --- a/runsc/boot/loader.go +++ b/runsc/boot/loader.go @@ -211,40 +211,41 @@ type Loader struct { // mu guards the fields below. mu sync.Mutex - // state is guarded by mu. + // +checklocks:mu state loaderState // sharedMounts holds VFS mounts that may be shared between containers within // the same pod. It is mapped by mount source. // - // sharedMounts is guarded by mu. + // +checklocks:mu sharedMounts map[string]*vfs.Mount // processes maps containers init process and invocation of exec. Root // processes are keyed with container ID and pid=0, while exec invocations // have the corresponding pid set. // - // processes is guarded by mu. + // +checklocks:mu processes map[execID]*execProcess // containerIDs store container names and IDs to assist with restore and container // naming when user didn't provide one. // // Mapping: name -> cid. - // containerIDs is guarded by mu. + // +checklocks:mu containerIDs map[string]string // containerSpecs stores container specs for each container in sandbox. // // Mapping: cid -> spec. - // containerSpecs is guarded by mu. + // +checklocks:mu containerSpecs map[string]*specs.Spec // portForwardProxies is a list of active port forwarding connections. // - // portForwardProxies is guarded by mu. + // +checklocks:mu portForwardProxies []*pf.Proxy + // +checklocks:mu saveFDs []*fd.FD } @@ -429,7 +430,7 @@ func New(args Args) (*Loader, error) { saveFDs: args.SaveFDs, } - containerName := l.registerContainerLocked(args.Spec, args.ID) + containerName := l.registerContainer(args.Spec, args.ID) l.root = containerInfo{ cid: args.ID, containerName: containerName, @@ -705,9 +706,11 @@ func (l *Loader) Destroy() { l.watchdog.Stop() ctx := l.k.SupervisorContext() + l.mu.Lock() for _, m := range l.sharedMounts { m.DecRef(ctx) } + l.mu.Unlock() // Stop the control server. This will indirectly stop any // long-running control operations that are in flight, e.g. @@ -1673,7 +1676,8 @@ func (l *Loader) threadGroupFromID(key execID) (*kernel.ThreadGroup, error) { // tryThreadGroupFromIDLocked returns the thread group for the given execution // ID. It may return nil in case the container has not started yet. Returns // error if execution ID is invalid or if the container cannot be found (maybe -// it has been deleted). Caller must hold 'mu'. +// it has been deleted). +// +checklocks:l.mu func (l *Loader) tryThreadGroupFromIDLocked(key execID) (*kernel.ThreadGroup, error) { ep, err := l.findProcessLocked(key) if err != nil { @@ -1685,7 +1689,8 @@ func (l *Loader) tryThreadGroupFromIDLocked(key execID) (*kernel.ThreadGroup, er // ttyFromIDLocked returns the TTY files for the given execution ID. It may // return nil in case the container has not started yet. Returns error if // execution ID is invalid or if the container cannot be found (maybe it has -// been deleted). Caller must hold 'mu'. +// been deleted). +// +checklocks:l.mu func (l *Loader) ttyFromIDLocked(key execID) (*host.TTYFileDescription, error) { ep, err := l.findProcessLocked(key) if err != nil { @@ -1868,6 +1873,7 @@ func (l *Loader) networkStats() ([]*NetworkInterface, error) { return stats, nil } +// +checklocks:l.mu func (l *Loader) findProcessLocked(key execID) (*execProcess, error) { ep := l.processes[key] if ep == nil { @@ -1883,6 +1889,7 @@ func (l *Loader) registerContainer(spec *specs.Spec, cid string) string { return l.registerContainerLocked(spec, cid) } +// +checklocks:l.mu func (l *Loader) registerContainerLocked(spec *specs.Spec, cid string) string { containerName := specutils.ContainerName(spec) if len(containerName) == 0 { diff --git a/runsc/boot/loader_test.go b/runsc/boot/loader_test.go index 57f91219d..414e553c8 100644 --- a/runsc/boot/loader_test.go +++ b/runsc/boot/loader_test.go @@ -480,6 +480,8 @@ func TestCreateMountNamespace(t *testing.T) { defer l.Destroy() defer loaderCleanup() + l.mu.Lock() + defer l.mu.Unlock() mntr := newContainerMounter(&l.root, l.k, l.mountHints, l.sharedMounts, "", l.sandboxID) ctx := l.k.SupervisorContext() creds := auth.NewRootCredentials(l.root.procArgs.Credentials.UserNamespace) diff --git a/runsc/boot/restore.go b/runsc/boot/restore.go index 0343f2c74..a1607c1a9 100644 --- a/runsc/boot/restore.go +++ b/runsc/boot/restore.go @@ -189,6 +189,12 @@ func (r *restorer) restore(l *Loader) error { ctx = context.WithValue(ctx, stack.CtxRestoreStack, oldStack) } + l.mu.Lock() + cu := cleanup.Make(func() { + l.mu.Unlock() + }) + defer cu.Clean() + fdmap := make(map[vfs.RestoreID]int) mfmap := make(map[string]*pgalloc.MemoryFile) for _, cont := range r.containers { @@ -233,12 +239,6 @@ func (r *restorer) restore(l *Loader) error { l.restore = true l.sandboxID = l.root.cid - l.mu.Lock() - cu := cleanup.Make(func() { - l.mu.Unlock() - }) - defer cu.Clean() - // Update all tasks in the system with their respective new container IDs. for _, task := range l.k.TaskSet().Root.Tasks() { oldCid := task.ContainerID()