diff --git a/pkg/sentry/fsimpl/cgroupfs/pids.go b/pkg/sentry/fsimpl/cgroupfs/pids.go index 5e1627e85..5f9c315da 100644 --- a/pkg/sentry/fsimpl/cgroupfs/pids.go +++ b/pkg/sentry/fsimpl/cgroupfs/pids.go @@ -161,14 +161,23 @@ func (c *pidsController) Leave(t *kernel.Task) { // PrepareMigrate implements controller.PrepareMigrate. func (c *pidsController) PrepareMigrate(t *kernel.Task, src controller) error { + srcC := src.(*pidsController) + srcC.mu.Lock() + defer srcC.mu.Unlock() + + if _, ok := srcC.pendingPool[t]; ok { + // Migrating task isn't fully initialized, return transient failure. + return linuxerr.EAGAIN + } + return nil } // CommitMigrate implements controller.CommitMigrate. // -// Migrations can cause a cgroup to exceed its limit. Migration can only be -// called for tasks with committed charges, as it is not possible to migrate a -// task prior to Enter. +// Migrations can cause a cgroup to exceed its limit. CommitMigrate can only be +// called for tasks with committed charges, PrepareMigrate will deny migrations +// prior to Enter. func (c *pidsController) CommitMigrate(t *kernel.Task, src controller) { // Note: The charge is allowed to exceed max on migration. The charge may // not exceed max when incurred due to a fork/clone, which will call diff --git a/pkg/sentry/kernel/task_cgroup.go b/pkg/sentry/kernel/task_cgroup.go index 2bd6d97f1..37084e7d9 100644 --- a/pkg/sentry/kernel/task_cgroup.go +++ b/pkg/sentry/kernel/task_cgroup.go @@ -89,6 +89,7 @@ func (t *Task) enterCgroupIfNotYetLocked(c Cgroup) { // LeaveCgroups removes t out from all its cgroups. func (t *Task) LeaveCgroups() { + t.tg.pidns.owner.mu.Lock() // Prevent migration. t.mu.Lock() cgs := t.cgroups t.cgroups = nil @@ -96,6 +97,8 @@ func (t *Task) LeaveCgroups() { c.Leave(t) } t.mu.Unlock() + t.tg.pidns.owner.mu.Unlock() + for c := range cgs { c.decRef() } @@ -229,33 +232,24 @@ func (t *Task) GenerateProcTaskCgroup(buf *bytes.Buffer) { } // +checklocks:t.mu -func (t *Task) chargeLocked(target *Task, ctl CgroupControllerType, res CgroupResourceType, value int64) (bool, uint32, error) { +func (t *Task) chargeLocked(target *Task, ctl CgroupControllerType, res CgroupResourceType, value int64) (bool, Cgroup, error) { // Due to the uniqueness of controllers on hierarchies, at most one cgroup // in t.cgroups will match. for c := range t.cgroups { err := c.Charge(target, c.Dentry, ctl, res, value) - return err == nil, c.HierarchyID(), err + if err == nil { + c.IncRef() + } + return err == nil, c, err } - return false, InvalidCgroupHierarchyID, nil + return false, Cgroup{}, nil } -// ChargeFor charges t's cgroup on behalf of some other task. -func (t *Task) ChargeFor(other *Task, ctl CgroupControllerType, res CgroupResourceType, value int64) (bool, uint32, error) { +// ChargeFor charges t's cgroup on behalf of some other task. Returns +// the cgroup that's charged if any. Returned cgroup has an extra ref +// that's transferred to the caller. +func (t *Task) ChargeFor(other *Task, ctl CgroupControllerType, res CgroupResourceType, value int64) (bool, Cgroup, error) { t.mu.Lock() defer t.mu.Unlock() return t.chargeLocked(other, ctl, res, value) } - -// ChargeForOnHierarchy is like ChargeFor, but only charges a cgroup with the -// matching hierarhcyID. This can be useful when reversing a charge across -// potential hierachy changes. -func (t *Task) ChargeForOnHierarchy(other *Task, hierarhcyID uint32, ctl CgroupControllerType, res CgroupResourceType, value int64) (bool, uint32, error) { - t.mu.Lock() - defer t.mu.Unlock() - for c := range t.cgroups { - if c.HierarchyID() == hierarhcyID { - return t.chargeLocked(other, ctl, res, value) - } - } - return false, InvalidCgroupHierarchyID, nil -} diff --git a/pkg/sentry/kernel/task_start.go b/pkg/sentry/kernel/task_start.go index 99c6911c0..e4d88f13c 100644 --- a/pkg/sentry/kernel/task_start.go +++ b/pkg/sentry/kernel/task_start.go @@ -176,7 +176,11 @@ func (ts *TaskSet) newTask(ctx context.Context, cfg *TaskConfig) (*Task, error) // We don't construct t.blockingTimer until Task.run(); see that function // for justification. - var cu cleanup.Cleanup + var ( + cg Cgroup + charged bool + cu cleanup.Cleanup + ) defer cu.Clean() // Reserve cgroup PIDs controller charge. This is either commited when the @@ -187,22 +191,16 @@ func (ts *TaskSet) newTask(ctx context.Context, cfg *TaskConfig) (*Task, error) // we skip charging the pids controller, as non-userspace task creation // bypasses pid limits. if srcT != nil { - var ( - charged bool - err error - hid uint32 - ) - if charged, hid, err = srcT.ChargeFor(t, CgroupControllerPIDs, CgroupResourcePID, 1); err != nil { + var err error + if charged, cg, err = srcT.ChargeFor(t, CgroupControllerPIDs, CgroupResourcePID, 1); err != nil { return nil, err } if charged { cu.Add(func() { - // Since ts.mu was dropped after the corresponding charge, the - // hierarchy referenced by hid may no longer exist. If so, this - // uncharge will be a no-op. - if _, _, err := srcT.ChargeForOnHierarchy(t, hid, CgroupControllerPIDs, CgroupResourcePID, -1); err != nil { + if err := cg.Charge(t, cg.Dentry, CgroupControllerPIDs, CgroupResourcePID, -1); err != nil { panic(fmt.Sprintf("Failed to clean up PIDs charge on task creation failure: %v", err)) } + cg.DecRef(ctx) // Ref from ChargeFor. }) } } @@ -241,6 +239,11 @@ func (ts *TaskSet) newTask(ctx context.Context, cfg *TaskConfig) (*Task, error) // srcT may be nil, in which case we default to root cgroups. t.EnterInitialCgroups(srcT) + cu.Release() + if charged { + cg.decRef() // Ref from ChargeFor. + } + if tg.leader == nil { // New thread group. tg.leader = t @@ -272,7 +275,6 @@ func (ts *TaskSet) newTask(ctx context.Context, cfg *TaskConfig) (*Task, error) // other pieces to be initialized as the task is used the context. t.p = cfg.Kernel.Platform.NewContext(t.AsyncContext()) - cu.Release() return t, nil }