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
This commit is contained in:
Ayush Ranjan
2024-08-16 10:38:23 -07:00
committed by gVisor bot
parent 6301795f75
commit afa4fef2c8
4 changed files with 38 additions and 17 deletions
+14 -2
View File
@@ -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 {
+16 -9
View File
@@ -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 {
+2
View File
@@ -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)
+6 -6
View File
@@ -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()