From 3ccdd9cc34c1d940b40bdc2eeabbd0b55cc886b0 Mon Sep 17 00:00:00 2001 From: Zeyad Gouda Date: Tue, 13 Aug 2024 12:43:48 +0300 Subject: [PATCH] sandbox/cgroup: support killing running snap apps This is an alternative implmentation to the userd approach in #14160. The userd approach will not work in the absense of user session daemon (i.e. ubuntu-core) or if a snap is started as root. The alternative algorithm kills the running apps in snapd directly in their cgroups. - many: address review comments (thanks @zyga) - add contexts support to FreezeSnapProcesses - add comment on possible race when killing processes in a v2 cgroup - for cgroups V2, only thaw on error - for cgroups V1, add comment explaining why we always need to thaw - update KillSnapProcesses to only send SIGKILL - replace deprecated os.IsNotExist with errors.Is - trim whitespace when reading freezer.state Signed-off-by: Zeyad Gouda --- cmd/snap-update-ns/common.go | 4 +- cmd/snap-update-ns/common_test.go | 3 +- cmd/snap-update-ns/system_test.go | 3 +- overlord/snapstate/backend_test.go | 1 + sandbox/cgroup/cgroup.go | 8 +- sandbox/cgroup/export_test.go | 29 ++++ sandbox/cgroup/freezer.go | 174 ++++++++++++-------- sandbox/cgroup/freezer_test.go | 40 +++-- sandbox/cgroup/kill.go | 122 ++++++++++++++ sandbox/cgroup/kill_test.go | 252 +++++++++++++++++++++++++++++ 10 files changed, 546 insertions(+), 90 deletions(-) create mode 100644 sandbox/cgroup/kill.go create mode 100644 sandbox/cgroup/kill_test.go diff --git a/cmd/snap-update-ns/common.go b/cmd/snap-update-ns/common.go index 805d843f41be..a552af807eab 100644 --- a/cmd/snap-update-ns/common.go +++ b/cmd/snap-update-ns/common.go @@ -20,6 +20,7 @@ package main import ( + "context" "fmt" "github.com/snapcore/snapd/cmd/snaplock" @@ -79,7 +80,8 @@ func (upCtx *CommonProfileUpdateContext) Lock() (func(), error) { // introduce a symlink that would cause us to mount something other // than what we expected). logger.Debugf("freezing processes of snap %q", instanceName) - if err := cgroup.FreezeSnapProcesses(instanceName); err != nil { + // TODO: Ideally we should use signal.NotifyContext + if err := cgroup.FreezeSnapProcesses(context.TODO(), instanceName); err != nil { // If we cannot freeze the processes we should drop the lock. lock.Close() return nil, err diff --git a/cmd/snap-update-ns/common_test.go b/cmd/snap-update-ns/common_test.go index 98b089deb81e..52066505da5a 100644 --- a/cmd/snap-update-ns/common_test.go +++ b/cmd/snap-update-ns/common_test.go @@ -21,6 +21,7 @@ package main_test import ( "bytes" + "context" "os" "path/filepath" @@ -55,7 +56,7 @@ func (s *commonSuite) TestInstanceName(c *C) { func (s *commonSuite) TestLock(c *C) { // Mock away real freezer code, allowing test code to return an error when freezing. var freezingError error - restore := cgroup.MockFreezing(func(string) error { return freezingError }, func(string) error { return nil }) + restore := cgroup.MockFreezing(func(context.Context, string) error { return freezingError }, func(string) error { return nil }) defer restore() // Mock system directories, we use the lock directory. dirs.SetRootDir(s.dir) diff --git a/cmd/snap-update-ns/system_test.go b/cmd/snap-update-ns/system_test.go index 066541a19c95..65136b3755ce 100644 --- a/cmd/snap-update-ns/system_test.go +++ b/cmd/snap-update-ns/system_test.go @@ -21,6 +21,7 @@ package main_test import ( "bytes" + "context" "os" "path/filepath" @@ -46,7 +47,7 @@ func (s *systemSuite) TestLockCgroup(c *C) { var frozen []string var thawed []string - happyFreeze := func(snapName string) error { + happyFreeze := func(ctx context.Context, snapName string) error { frozen = append(frozen, snapName) return nil } diff --git a/overlord/snapstate/backend_test.go b/overlord/snapstate/backend_test.go index f7113a9dd78a..3d8fbe756fe6 100644 --- a/overlord/snapstate/backend_test.go +++ b/overlord/snapstate/backend_test.go @@ -1295,6 +1295,7 @@ func (f *fakeSnappyBackend) StartServices(svcs []*snap.AppInfo, disabledSvcs *wr } func (f *fakeSnappyBackend) StopServices(svcs []*snap.AppInfo, reason snap.ServiceStopReason, meter progress.Meter, tm timings.Measurer) error { + meter.Notify("stop-services") f.appendOp(&fakeOp{ op: fmt.Sprintf("stop-snap-services:%s", reason), path: svcSnapMountDir(svcs), diff --git a/sandbox/cgroup/cgroup.go b/sandbox/cgroup/cgroup.go index 097d361ee5f4..091dd3d74248 100644 --- a/sandbox/cgroup/cgroup.go +++ b/sandbox/cgroup/cgroup.go @@ -73,9 +73,13 @@ func init() { func pickVersionSpecificImpl() { switch probeVersion { case V1: - pickFreezerV1Impl() + FreezeSnapProcesses = freezeSnapProcessesImplV1 + ThawSnapProcesses = thawSnapProcessesImplV1 + KillSnapProcesses = killSnapProcessesImplV1 case V2: - pickFreezerV2Impl() + FreezeSnapProcesses = freezeSnapProcessesImplV2 + ThawSnapProcesses = thawSnapProcessesImplV2 + KillSnapProcesses = killSnapProcessesImplV2 } } diff --git a/sandbox/cgroup/export_test.go b/sandbox/cgroup/export_test.go index 641bb7ea7f88..bfe86c820fd6 100644 --- a/sandbox/cgroup/export_test.go +++ b/sandbox/cgroup/export_test.go @@ -20,6 +20,7 @@ package cgroup import ( "context" + "syscall" "time" "github.com/godbus/dbus" @@ -135,3 +136,31 @@ func (iw *inotifyWatcher) MonitorDelete(folders []string, name string, channel c } var NewInotifyWatcher = newInotifyWatcher + +func MockFreezeSnapProcessesImplV1(fn func(ctx context.Context, snapName string) error) (restore func()) { + return testutil.Mock(&freezeSnapProcessesImplV1, fn) +} + +func MockThawSnapProcessesImplV1(fn func(snapName string) error) (restore func()) { + return testutil.Mock(&thawSnapProcessesImplV1, fn) +} + +func MockSyscallKill(fn func(pid int, sig syscall.Signal) error) (restore func()) { + return testutil.Mock(&syscallKill, fn) +} + +func MockFreezeOneV2(fn func(ctx context.Context, dir string) error) (restore func()) { + return testutil.Mock(&freezeOneV2, fn) +} + +func MockThawOneV2(fn func(dir string) error) (restore func()) { + return testutil.Mock(&thawOneV2, fn) +} + +func MockFreezePulseDelay(t time.Duration) (restore func()) { + return testutil.Mock(&freezePulseDelay, t) +} + +func MockOsReadFile(fn func(name string) ([]byte, error)) (restore func()) { + return testutil.Mock(&osReadFile, fn) +} diff --git a/sandbox/cgroup/freezer.go b/sandbox/cgroup/freezer.go index 0dbaded8582f..30d1bd1f3e80 100644 --- a/sandbox/cgroup/freezer.go +++ b/sandbox/cgroup/freezer.go @@ -21,7 +21,10 @@ package cgroup import ( "bytes" + "context" + "errors" "fmt" + "io/fs" "os" "path/filepath" "strings" @@ -33,8 +36,12 @@ import ( ) const defaultFreezerCgroupV1Dir = "/sys/fs/cgroup/freezer" +const maxFreezeTimeout = 3000 * time.Millisecond var freezerCgroupV1Dir = defaultFreezerCgroupV1Dir +var freezePulseDelay = 100 * time.Millisecond + +var osReadFile = os.ReadFile func init() { dirs.AddRootDirCallback(func(root string) { @@ -42,24 +49,14 @@ func init() { }) } -func pickFreezerV1Impl() { - FreezeSnapProcesses = freezeSnapProcessesImplV1 - ThawSnapProcesses = thawSnapProcessesImplV1 -} - -func pickFreezerV2Impl() { - FreezeSnapProcesses = freezeSnapProcessesImplV2 - ThawSnapProcesses = thawSnapProcessesImplV2 -} - // FreezeSnapProcesses suspends execution of all the processes belonging to // a given snap. Processes remain frozen until ThawSnapProcesses is called, // care must be taken not to freezer processes indefinitely. // // The freeze operation is not instant. Once commenced it proceeds // asynchronously. Internally the function waits for the freezing to complete -// in at most 3000ms. If this time is insufficient then the processes are -// thawed and an error is returned. +// in at most maxFreezeTimeout or if the passed context is cancelled. If this +// time is insufficient then the processes are thawed and an error is returned. // // A correct implementation is picked depending on cgroup v1 or v2 use in the // system. When cgroup v1 is detected, the call will directly act on the freezer @@ -67,7 +64,9 @@ func pickFreezerV2Impl() { // act on all tracking groups of a snap. // // This operation can be mocked with MockFreezing -var FreezeSnapProcesses = freezeSnapProcessesImplV1 +var FreezeSnapProcesses = func(ctx context.Context, snapName string) error { + return errors.New("FreezeSnapProcesses not implemented") +} // ThawSnapProcesses resumes execution of all processes belonging to a given snap. // @@ -77,41 +76,54 @@ var FreezeSnapProcesses = freezeSnapProcessesImplV1 // act on all tracking groups of a snap. // // This operation can be mocked with MockFreezing -var ThawSnapProcesses = thawSnapProcessesImplV1 +var ThawSnapProcesses = func(snapName string) error { + return errors.New("ThawSnapProcesses not implemented") +} // freezeSnapProcessesImplV1 freezes all the processes originating from the given snap. // Processes are frozen regardless of which particular snap application they // originate from. -func freezeSnapProcessesImplV1(snapName string) error { +var freezeSnapProcessesImplV1 = func(ctx context.Context, snapName string) error { fname := filepath.Join(freezerCgroupV1Dir, fmt.Sprintf("snap.%s", snapName), "freezer.state") - if err := os.WriteFile(fname, []byte("FROZEN"), 0644); err != nil && os.IsNotExist(err) { + if err := os.WriteFile(fname, []byte("FROZEN"), 0644); errors.Is(err, fs.ErrNotExist) { // When there's no freezer cgroup we don't have to freeze anything. // This can happen when no process belonging to a given snap has been // started yet. return nil } else if err != nil { - return fmt.Errorf("cannot freeze processes of snap %q, %v", snapName, err) + return fmt.Errorf("cannot freeze processes of snap %q, %w", snapName, err) } - for i := 0; i < 30; i++ { - data, err := os.ReadFile(fname) + + // Add an upper bound to the timeout if cgroup is stuck at freeze state + ctxWithTimeout, cancel := context.WithTimeout(ctx, maxFreezeTimeout) + defer cancel() + ticker := time.NewTicker(freezePulseDelay) + // The ticker.Stop() can be removed starting 1.23 where the garbage collector can recover + // unreferenced tickers, even if they haven't been stopped. + defer ticker.Stop() + for { + data, err := osReadFile(fname) if err != nil { - return fmt.Errorf("cannot determine the freeze state of processes of snap %q, %v", snapName, err) + return fmt.Errorf("cannot determine the freeze state of processes of snap %q, %w", snapName, err) } - // If the cgroup is still freezing then wait a moment and try again. - if bytes.Equal(data, []byte("FREEZING")) { - time.Sleep(100 * time.Millisecond) - continue + // If the cgroup is frozen then we are done + if bytes.Equal(bytes.TrimSpace(data), []byte("FROZEN")) { + return nil + } + // Timeout or add a bit of delay + select { + case <-ctxWithTimeout.Done(): + // If we got here then we timed out after seeing FREEZING for too long. + ThawSnapProcesses(snapName) // ignore the error, this is best-effort. + return fmt.Errorf("cannot finish freezing processes of snap %q (freezer state: %s): %w", snapName, data, ctxWithTimeout.Err()) + case <-ticker.C: } - return nil } - // If we got here then we timed out after seeing FREEZING for too long. - ThawSnapProcesses(snapName) // ignore the error, this is best-effort. - return fmt.Errorf("cannot finish freezing processes of snap %q", snapName) } -func thawSnapProcessesImplV1(snapName string) error { +var thawSnapProcessesImplV1 = func(snapName string) error { fname := filepath.Join(freezerCgroupV1Dir, fmt.Sprintf("snap.%s", snapName), "freezer.state") - if err := os.WriteFile(fname, []byte("THAWED"), 0644); err != nil && os.IsNotExist(err) { + if err := os.WriteFile(fname, []byte("THAWED"), 0644); err != nil && errors.Is(err, fs.ErrNotExist) { // When there's no freezer cgroup we don't have to thaw anything. // This can happen when no process belonging to a given snap has been // started yet. @@ -164,8 +176,8 @@ func applyToSnap(snapName string, action func(groupName string) error, skipError // writeExistingFile can be used as a drop-in replacement for os.WriteFile, // but does not create a file when it does not exist -func writeExistingFile(where string, data []byte, mode os.FileMode) error { - f, err := os.OpenFile(where, os.O_WRONLY|os.O_TRUNC, mode) +func writeExistingFile(where string, data []byte) error { + f, err := os.OpenFile(where, os.O_WRONLY|os.O_TRUNC, 0644) if err != nil { return err } @@ -178,10 +190,55 @@ func writeExistingFile(where string, data []byte, mode os.FileMode) error { return errC } +var freezeOneV2 = func(ctx context.Context, dir string) error { + groupName := filepath.Base(dir) + fname := filepath.Join(dir, "cgroup.freeze") + if err := writeExistingFile(fname, []byte("1")); err != nil { + if errors.Is(err, fs.ErrNotExist) { + // the group may be gone already + return nil + } + return fmt.Errorf("cannot freeze processes in group %q: %w", groupName, err) + } + + // Add an upper bound to the timeout if cgroup is stuck at freeze state + ctxWithTimeout, cancel := context.WithTimeout(ctx, maxFreezeTimeout) + defer cancel() + ticker := time.NewTicker(freezePulseDelay) + // ticker.Stop() can be removed starting 1.23 where the garbage collector can recover + // unreferenced tickers, even if they haven't been stopped. + defer ticker.Stop() + for { + data, err := os.ReadFile(fname) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + // group may be gone + return nil + } + return fmt.Errorf("cannot determine the freeze state of processes in group %q: %w", groupName, err) + } + // If the cgroup is still freezing then wait a moment and try again. + if bytes.Equal(bytes.TrimSpace(data), []byte("1")) { + // we're done + return nil + } + // timeout or add a bit of delay + select { + case <-ctxWithTimeout.Done(): + return fmt.Errorf("cannot freeze processes in group %q: %w", groupName, ctxWithTimeout.Err()) + case <-ticker.C: + } + } +} + +func skipErrNotExist(err error) bool { + return errors.Is(err, fs.ErrNotExist) +} + // freezeSnapProcessesImplV2 freezes all the processes originating from the // given snap. Processes are frozen regardless of which particular snap // application they originate from. -func freezeSnapProcessesImplV2(snapName string) error { +func freezeSnapProcessesImplV2(ctx context.Context, snapName string) error { // in case of v2, the process calling this code, (eg. snap-update-ns) // may already be part of the trackign cgroup for particular snap, care // must be taken to not freeze ourselves @@ -196,35 +253,10 @@ func freezeSnapProcessesImplV2(snapName string) error { logger.Debugf("freeze, skipping own group %v", dir) return nil } - fname := filepath.Join(dir, "cgroup.freeze") - if err := writeExistingFile(fname, []byte("1"), 0644); err != nil { - if os.IsNotExist(err) { - // the group may be gone already - return nil - } - return fmt.Errorf("cannot freeze processes of snap %q, %v", snapName, err) - } - for i := 0; i < 30; i++ { - data, err := os.ReadFile(fname) - if err != nil { - if os.IsNotExist(err) { - // group may be gone - return nil - } - return fmt.Errorf("cannot determine the freeze state of processes of snap %q, %v", snapName, err) - } - // If the cgroup is still freezing then wait a moment and try again. - if bytes.Equal(bytes.TrimSpace(data), []byte("1")) { - // we're done - return nil - } - // add a bit of delay - time.Sleep(100 * time.Millisecond) - } - return fmt.Errorf("cannot freeze processes of snap %q in group %v", snapName, filepath.Base(dir)) + return freezeOneV2(ctx, dir) } // freeze, skipping ENOENT errors - err = applyToSnap(snapName, freezeOne, os.IsNotExist) + err = applyToSnap(snapName, freezeOne, skipErrNotExist) if err == nil { return nil } @@ -234,7 +266,15 @@ func freezeSnapProcessesImplV2(snapName string) error { // ignore errors when thawing processes, this is best-effort. alwaysSkipError := func(_ error) bool { return true } thawSnapProcessesV2(snapName, alwaysSkipError) - return fmt.Errorf("cannot finish freezing processes of snap %q: %v", snapName, err) + return fmt.Errorf("cannot finish freezing processes of snap %q: %w", snapName, err) +} + +var thawOneV2 = func(dir string) error { + fname := filepath.Join(dir, "cgroup.freeze") + if err := writeExistingFile(fname, []byte("0")); err != nil && !errors.Is(err, fs.ErrNotExist) { + return err + } + return nil } func thawSnapProcessesV2(snapName string, skipError func(error) bool) error { @@ -242,12 +282,8 @@ func thawSnapProcessesV2(snapName string, skipError func(error) bool) error { return fmt.Errorf("internal error: skip error is nil") } thawOne := func(dir string) error { - fname := filepath.Join(dir, "cgroup.freeze") - if err := writeExistingFile(fname, []byte("0"), 0644); err != nil && os.IsNotExist(err) { - // the group may be gone already - return nil - } else if err != nil && !skipError(err) { - return fmt.Errorf("cannot thaw processes of snap %q, %v", snapName, err) + if err := thawOneV2(dir); err != nil && !skipError(err) { + return fmt.Errorf("cannot thaw processes of snap %q, %w", snapName, err) } return nil } @@ -256,11 +292,11 @@ func thawSnapProcessesV2(snapName string, skipError func(error) bool) error { func thawSnapProcessesImplV2(snapName string) error { // thaw skipping ENOENT errors - return thawSnapProcessesV2(snapName, os.IsNotExist) + return thawSnapProcessesV2(snapName, skipErrNotExist) } // MockFreezing replaces the real implementation of freeze and thaw. -func MockFreezing(freeze, thaw func(snapName string) error) (restore func()) { +func MockFreezing(freeze func(ctx context.Context, snapName string) error, thaw func(snapName string) error) (restore func()) { oldFreeze := FreezeSnapProcesses oldThaw := ThawSnapProcesses diff --git a/sandbox/cgroup/freezer_test.go b/sandbox/cgroup/freezer_test.go index 3f7bea54c802..8617566ab6c7 100644 --- a/sandbox/cgroup/freezer_test.go +++ b/sandbox/cgroup/freezer_test.go @@ -20,6 +20,7 @@ package cgroup_test import ( + "context" "fmt" "os" "path/filepath" @@ -35,6 +36,7 @@ import ( type freezerV1Suite struct{} +// TODO: Add SetUpTest for common setup var _ = Suite(&freezerV1Suite{}) func (s *freezerV1Suite) TestFreezeSnapProcessesV1(c *C) { @@ -47,20 +49,26 @@ func (s *freezerV1Suite) TestFreezeSnapProcessesV1(c *C) { f := filepath.Join(p, "freezer.state") // freezer.state file of the cgroup // When the freezer cgroup filesystem doesn't exist we do nothing at all. - c.Assert(cgroup.FreezeSnapProcesses(n), IsNil) + c.Assert(cgroup.FreezeSnapProcesses(context.TODO(), n), IsNil) _, err := os.Stat(f) c.Assert(os.IsNotExist(err), Equals, true) // When the freezer cgroup filesystem exists but the particular cgroup // doesn't exist we don nothing at all. c.Assert(os.MkdirAll(cgroup.FreezerCgroupV1Dir(), 0755), IsNil) - c.Assert(cgroup.FreezeSnapProcesses(n), IsNil) + c.Assert(cgroup.FreezeSnapProcesses(context.TODO(), n), IsNil) _, err = os.Stat(f) c.Assert(os.IsNotExist(err), Equals, true) + restore := cgroup.MockOsReadFile(func(name string) ([]byte, error) { + // Test that whitespace is trimmed when parsing freezer.state + return []byte("FROZEN\n"), nil + }) + defer restore() + // When the cgroup exists we write FROZEN the freezer.state file. c.Assert(os.MkdirAll(p, 0755), IsNil) - c.Assert(cgroup.FreezeSnapProcesses(n), IsNil) + c.Assert(cgroup.FreezeSnapProcesses(context.TODO(), n), IsNil) _, err = os.Stat(f) c.Assert(err, IsNil) c.Assert(f, testutil.FileEquals, `FROZEN`) @@ -129,7 +137,7 @@ func (s *freezerV2Suite) TestFreezeSnapProcessesV2OtherGroups(c *C) { // freezing needs to inspect our own cgroup, which will fail without // proper mocking - err := cgroup.FreezeSnapProcesses("foo") + err := cgroup.FreezeSnapProcesses(context.TODO(), "foo") c.Check(err, ErrorMatches, fmt.Sprintf("open %s/proc/%v/cgroup: no such file or directory", dirs.GlobalRootDir, pid)) procPidCgroup := filepath.Join(dirs.GlobalRootDir, fmt.Sprintf("proc/%v/cgroup", pid)) @@ -137,7 +145,7 @@ func (s *freezerV2Suite) TestFreezeSnapProcessesV2OtherGroups(c *C) { c.Assert(os.WriteFile(procPidCgroup, []byte("0::/foo/bar"), 0755), IsNil) // When the freezer cgroup filesystem doesn't exist we do nothing at all. - c.Assert(cgroup.FreezeSnapProcesses("foo"), IsNil) + c.Assert(cgroup.FreezeSnapProcesses(context.TODO(), "foo"), IsNil) for _, p := range []string{ g1, g2, g3, g4, @@ -156,7 +164,7 @@ func (s *freezerV2Suite) TestFreezeSnapProcessesV2OtherGroups(c *C) { c.Assert(os.WriteFile(p, []byte("0"), 0644), IsNil) } - c.Assert(cgroup.FreezeSnapProcesses("foo"), IsNil) + c.Assert(cgroup.FreezeSnapProcesses(context.TODO(), "foo"), IsNil) for _, p := range []string{g1, g2, g3, g4} { c.Check(p, testutil.FileEquals, "1") } @@ -168,7 +176,7 @@ func (s *freezerV2Suite) TestFreezeSnapProcessesV2OtherGroups(c *C) { } // all groups are 'frozen', repeating the action does not break anything - c.Assert(cgroup.FreezeSnapProcesses("foo"), IsNil) + c.Assert(cgroup.FreezeSnapProcesses(context.TODO(), "foo"), IsNil) for _, p := range []string{g1, g2, g3, g4} { c.Check(p, testutil.FileEquals, "1") } @@ -183,7 +191,7 @@ func (s *freezerV2Suite) TestFreezeSnapProcessesV2OtherGroups(c *C) { for _, p := range []string{g2, g3} { c.Assert(os.WriteFile(p, []byte("0"), 0644), IsNil) } - c.Assert(cgroup.FreezeSnapProcesses("foo"), IsNil) + c.Assert(cgroup.FreezeSnapProcesses(context.TODO(), "foo"), IsNil) // all are frozen again for _, p := range []string{g1, g2, g3, g4} { c.Check(p, testutil.FileEquals, "1") @@ -211,7 +219,7 @@ func (s *freezerV2Suite) TestFreezeSnapProcessesV2OwnGroup(c *C) { // freezing needs to inspect our own cgroup, which will fail without // proper mocking - err := cgroup.FreezeSnapProcesses("foo") + err := cgroup.FreezeSnapProcesses(context.TODO(), "foo") c.Check(err, ErrorMatches, fmt.Sprintf("open %s/proc/%v/cgroup: no such file or directory", dirs.GlobalRootDir, pid)) procPidCgroup := filepath.Join(dirs.GlobalRootDir, fmt.Sprintf("proc/%v/cgroup", pid)) @@ -224,7 +232,7 @@ func (s *freezerV2Suite) TestFreezeSnapProcessesV2OwnGroup(c *C) { c.Assert(os.WriteFile(p, []byte("0"), 0644), IsNil) } - c.Assert(cgroup.FreezeSnapProcesses("foo"), IsNil) + c.Assert(cgroup.FreezeSnapProcesses(context.TODO(), "foo"), IsNil) // our own group is not frozen c.Assert(gOwn, testutil.FileEquals, "0") // canaries have not been changed @@ -346,7 +354,7 @@ func (s *freezerV2Suite) TestFreezeThawSnapProcessesV2ErrWalking(c *C) { defer os.Chmod(filepath.Dir(g), 0755) // freeze tries thawing on errors, so we'll observe both errors - err := cgroup.FreezeSnapProcesses("foo") + err := cgroup.FreezeSnapProcesses(context.TODO(), "foo") // go 1.10+ slightly changed the order of calls in filepath.Walk(), make // sure the error check matches both c.Check(err, ErrorMatches, `cannot finish freezing processes of snap "foo":( cannot freeze processes of snap "foo",)? open .*/sys/fs/cgroup/system.slice/snap.foo.app.1234.1234.1234.scope(/cgroup.freeze)?: permission denied`) @@ -358,8 +366,8 @@ func (s *freezerV2Suite) TestFreezeThawSnapProcessesV2ErrWalking(c *C) { c.Assert(os.Chmod(filepath.Dir(g), 0755), IsNil) c.Assert(os.Chmod(g, 0000), IsNil) // other group was unfrozen - err = cgroup.FreezeSnapProcesses("foo") - c.Check(err, ErrorMatches, `cannot finish freezing processes of snap "foo": cannot freeze processes of snap "foo", open .*/sys/fs/cgroup/system.slice/snap.foo.app.1234.1234.1234.scope/cgroup.freeze: permission denied`) + err = cgroup.FreezeSnapProcesses(context.TODO(), "foo") + c.Check(err, ErrorMatches, `cannot finish freezing processes of snap "foo": cannot freeze processes in group "snap.foo.app.1234-1234-1234.scope": open .*/sys/fs/cgroup/system.slice/snap.foo.app.1234.1234.1234.scope/cgroup.freeze: permission denied`) // other group was unfrozen c.Check(gUnfreeze, testutil.FileEquals, "0") @@ -374,9 +382,9 @@ func (s *freezerV2Suite) TestFreezeThawSnapProcessesV2ErrWalking(c *C) { c.Assert(os.Chmod(filepath.Dir(gUnfreeze), 0000), IsNil) defer os.Chmod(filepath.Dir(gUnfreeze), 0755) - err = cgroup.FreezeSnapProcesses("foo") + err = cgroup.FreezeSnapProcesses(context.TODO(), "foo") // but the unfreeze errors are ignored anyuway - c.Check(err, ErrorMatches, `cannot finish freezing processes of snap "foo": cannot freeze processes of snap "foo", open .*/sys/fs/cgroup/system.slice/snap.foo.app.1234.1234.1234.scope/cgroup.freeze: permission denied`) + c.Check(err, ErrorMatches, `cannot finish freezing processes of snap "foo": cannot freeze processes in group "snap.foo.app.1234-1234-1234.scope": open .*/sys/fs/cgroup/system.slice/snap.foo.app.1234.1234.1234.scope/cgroup.freeze: permission denied`) // the other group is unmodified os.Chmod(filepath.Dir(gUnfreeze), 0755) c.Check(gUnfreeze, testutil.FileEquals, "1") @@ -400,7 +408,7 @@ func (s *freezerV2Suite) TestFreezeThawSnapProcessesV2ErrNotFound(c *C) { c.Assert(os.MkdirAll(filepath.Dir(g1), 0755), IsNil) c.Assert(os.MkdirAll(filepath.Dir(g2), 0755), IsNil) - err := cgroup.FreezeSnapProcesses("foo") + err := cgroup.FreezeSnapProcesses(context.TODO(), "foo") c.Assert(err, IsNil) c.Check(g1, testutil.FileAbsent) diff --git a/sandbox/cgroup/kill.go b/sandbox/cgroup/kill.go new file mode 100644 index 000000000000..cb67440c7951 --- /dev/null +++ b/sandbox/cgroup/kill.go @@ -0,0 +1,122 @@ +// -*- Mode: Go; indent-tabs-mode: t -*- + +/* + * Copyright (C) 2024 Canonical Ltd + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 3 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + */ + +package cgroup + +import ( + "context" + "errors" + "fmt" + "io/fs" + "path/filepath" + "syscall" +) + +// KillSnapProcesses sends a signal to all the processes belonging to +// a given snap. +// +// A correct implementation is picked depending on cgroup v1 or v2 use in the +// system. When cgroup v1 is detected, the call will directly act on the freezer +// group created when a snap process was started, while with v2 the call will +// act on all tracking groups of a snap. +// +// XXX: It is important to note that the implementation for v2 is racy because +// processes can be killed externally even when the cgroup is frozen and have +// their pid reused. Also, v2 freezing support is a no-op on kernels before +// 5.2 which means new processes can keep getting spawned while killing +// pids read earlier. +var KillSnapProcesses = func(ctx context.Context, snapName string) error { + return errors.New("KillSnapProcesses not implemented") +} + +var syscallKill = syscall.Kill + +// killProcessesInCgroup sends signal to all the processes belonging to +// passed cgroup directory. +// +// The caller is responsible for making sure that pids are not reused +// after reading `cgroup.procs` to avoid TOCTOU. +func killProcessesInCgroup(dir string, signal syscall.Signal) error { + pids, err := pidsInFile(filepath.Join(dir, "cgroup.procs")) + if err != nil { + return err + } + + var firstErr error + for _, pid := range pids { + pidNotFoundErr := syscall.ESRCH + if err := syscallKill(pid, signal); err != nil && !errors.As(err, &pidNotFoundErr) && firstErr == nil { + firstErr = err + } + } + + return firstErr +} + +func killSnapProcessesImplV1(ctx context.Context, snapName string) error { + if err := freezeSnapProcessesImplV1(ctx, snapName); err != nil { + return err + } + // For V1, SIGKILL on a frozen cgroup will not take effect + // until the cgroup is thawed + defer thawSnapProcessesImplV1(snapName) + + return killProcessesInCgroup(filepath.Join(freezerCgroupV1Dir, fmt.Sprintf("snap.%s", snapName)), syscall.SIGKILL) +} + +// XXX: killSnapProcessesImplV2 is racy to varying degrees depending on the kernel +// version. +// +// 1. Cgroup v2 freezer was only available since Linux 5.2 so freezing is a no-op before 5.2 which allows processes to keep forking. +// 2. Freezing does not put processes in an uninterruptable sleep unlike v1, so they can be killed externally and have their pid reused. +// 3. `cgroup.kill` was introduced in Linux 5.14 and solves the above issues as it kills the cgroup processes atomically. +func killSnapProcessesImplV2(ctx context.Context, snapName string) error { + killCgroupProcs := func(dir string) error { + // Use cgroup.kill if it exists (requires linux 5.14+) + err := writeExistingFile(filepath.Join(dir, "cgroup.kill"), []byte("1")) + if err == nil || !errors.Is(err, fs.ErrNotExist) { + return err + } + // Fallback to classic freeze/kill/thaw if cgroup.kill doesn't exist + + if err := freezeOneV2(ctx, dir); err != nil { + return err + } + if err := killProcessesInCgroup(dir, syscall.SIGKILL); err != nil { + // Thaw on error to avoid keeping cgroup stuck + thawOneV2(dir) // ignore the error, this is best-effort + return err + } + return nil + } + + var firstErr error + skipError := func(err error) bool { + if !errors.Is(err, fs.ErrNotExist) && firstErr == nil { + firstErr = err + } + return true + } + + if err := applyToSnap(snapName, killCgroupProcs, skipError); err != nil { + return err + } + + return firstErr +} diff --git a/sandbox/cgroup/kill_test.go b/sandbox/cgroup/kill_test.go new file mode 100644 index 000000000000..461d07db65e5 --- /dev/null +++ b/sandbox/cgroup/kill_test.go @@ -0,0 +1,252 @@ +// -*- Mode: Go; indent-tabs-mode: t -*- + +/* + * Copyright (C) 2024 Canonical Ltd + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 3 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + */ +package cgroup_test + +import ( + "context" + "fmt" + "os" + "path/filepath" + "syscall" + "time" + + . "gopkg.in/check.v1" + + "github.com/snapcore/snapd/dirs" + "github.com/snapcore/snapd/sandbox/cgroup" + "github.com/snapcore/snapd/testutil" +) + +type killSuite struct { + testutil.BaseTest + rootDir string + + ops []string +} + +var _ = Suite(&killSuite{}) + +func (s *killSuite) SetUpTest(c *C) { + s.BaseTest.SetUpTest(c) + + s.rootDir = c.MkDir() + dirs.SetRootDir(s.rootDir) + s.AddCleanup(func() { dirs.SetRootDir("") }) + + restore := cgroup.MockFreezePulseDelay(time.Nanosecond) + s.AddCleanup(restore) + + restore = cgroup.MockFreezeSnapProcessesImplV1(func(ctx context.Context, snapName string) error { + s.ops = append(s.ops, "freeze-snap-processes-v1:"+snapName) + return nil + }) + s.AddCleanup(restore) + restore = cgroup.MockFreezeOneV2(func(ctx context.Context, dir string) error { + s.ops = append(s.ops, "freeze-one-v2:"+filepath.Base(dir)) + return nil + }) + s.AddCleanup(restore) + + restore = cgroup.MockThawSnapProcessesImplV1(func(snapName string) error { + s.ops = append(s.ops, "thaw-snap-processes-v1:"+snapName) + return nil + }) + s.AddCleanup(restore) + restore = cgroup.MockThawOneV2(func(dir string) error { + s.ops = append(s.ops, "thaw-one-v2:"+filepath.Base(dir)) + return nil + }) + s.AddCleanup(restore) + + restore = cgroup.MockSyscallKill(func(pid int, sig syscall.Signal) error { + s.ops = append(s.ops, fmt.Sprintf("kill-pid:%d, signal:%d", pid, sig)) + return nil + }) + s.AddCleanup(restore) + + s.AddCleanup(s.clearOps) +} + +func (s *killSuite) clearOps() { + s.ops = nil +} + +func (s *killSuite) TestKillSnapProcessesV1(c *C) { + restore := cgroup.MockVersion(cgroup.V1, nil) + defer restore() + + snapName := "foo" + cg := filepath.Join(cgroup.FreezerCgroupV1Dir(), fmt.Sprintf("snap.%s", snapName)) + procs := filepath.Join(cg, "cgroup.procs") + + c.Assert(os.MkdirAll(cg, 0755), IsNil) + c.Assert(os.WriteFile(procs, nil, 0644), IsNil) + + // When no pids exist in cgroup.procs, do nothing + c.Assert(cgroup.KillSnapProcesses(context.TODO(), snapName), IsNil) + c.Assert(s.ops, DeepEquals, []string{ + "freeze-snap-processes-v1:foo", + "thaw-snap-processes-v1:foo", + }) + // Clear logged ops for following checks + s.clearOps() + + // Now mock running pids + c.Assert(os.WriteFile(procs, []byte("3\n1\n2"), 0644), IsNil) + c.Assert(cgroup.KillSnapProcesses(context.TODO(), snapName), IsNil) + c.Assert(s.ops, DeepEquals, []string{ + "freeze-snap-processes-v1:foo", + "kill-pid:3, signal:9", + "kill-pid:1, signal:9", + "kill-pid:2, signal:9", + "thaw-snap-processes-v1:foo", + }) +} + +func (s *killSuite) testKillSnapProcessesV2(c *C, cgroupKillSupported bool) { + restore := cgroup.MockVersion(cgroup.V2, nil) + defer restore() + + scopesToProcs := map[string]string{ + "snap.foo.app-1.1234-1234-1234.scope": "1\n2\n3", + "snap.foo.app-1.no-pids.scope": "", + "snap.foo.app-2.some-scope.scope": "9\n8\n7", + } + + for scope, pids := range scopesToProcs { + cg := filepath.Join(dirs.GlobalRootDir, "/sys/fs/cgroup/system.slice", scope) + procs := filepath.Join(cg, "cgroup.procs") + c.Assert(os.MkdirAll(cg, 0755), IsNil) + c.Assert(os.WriteFile(procs, []byte(pids), 0644), IsNil) + if cgroupKillSupported { + c.Assert(os.WriteFile(filepath.Join(cg, "cgroup.kill"), []byte(""), 0644), IsNil) + } + } + + c.Assert(cgroup.KillSnapProcesses(context.TODO(), "foo"), IsNil) + + if cgroupKillSupported { + for scope := range scopesToProcs { + cgKill := filepath.Join(dirs.GlobalRootDir, "/sys/fs/cgroup/system.slice", scope, "cgroup.kill") + // "1" was written to cgroup.kill + c.Assert(cgKill, testutil.FileEquals, "1") + } + // Didn't fallback to classic implementation + c.Assert(s.ops, IsNil) + } else { + for scope := range scopesToProcs { + cgKill := filepath.Join(dirs.GlobalRootDir, "/sys/fs/cgroup/system.slice", scope, "cgroup.kill") + c.Assert(cgKill, testutil.FileAbsent) + } + c.Assert(s.ops, DeepEquals, []string{ + // Kill first cgroup + "freeze-one-v2:snap.foo.app-1.1234-1234-1234.scope", + "kill-pid:1, signal:9", + "kill-pid:2, signal:9", + "kill-pid:3, signal:9", + // No pids to kill in second cgroup + "freeze-one-v2:snap.foo.app-1.no-pids.scope", + // Kill third cgroup + "freeze-one-v2:snap.foo.app-2.some-scope.scope", + "kill-pid:9, signal:9", + "kill-pid:8, signal:9", + "kill-pid:7, signal:9", + }) + } +} + +func (s *killSuite) TestKillSnapProcessesV2(c *C) { + const cgroupKillSupported = false + s.testKillSnapProcessesV2(c, cgroupKillSupported) +} + +func (s *killSuite) TestKillSnapProcessesV2CgKillSupported(c *C) { + // cgroup.kill requires linux 5.14+ + const cgroupKillSupported = true + s.testKillSnapProcessesV2(c, cgroupKillSupported) +} + +func (s *killSuite) testKillSnapProcessesError(c *C, cgVersion int) { + restore := cgroup.MockVersion(cgVersion, nil) + defer restore() + + if cgVersion == cgroup.V1 { + procs := filepath.Join(cgroup.FreezerCgroupV1Dir(), "snap.foo", "cgroup.procs") + c.Assert(os.MkdirAll(filepath.Dir(procs), 0755), IsNil) + c.Assert(os.WriteFile(procs, []byte("1\n2\n3\n4"), 0644), IsNil) + } else { + scopesToProcs := map[string]string{ + "snap.foo.app-1.1234-1234-1234.scope": "1", + "snap.foo.app-1.no-pids.scope": "2\n3", + "snap.foo.app-2.some-scope.scope": "4", + } + for scope, pids := range scopesToProcs { + procs := filepath.Join(dirs.GlobalRootDir, "/sys/fs/cgroup/system.slice", scope, "cgroup.procs") + c.Assert(os.MkdirAll(filepath.Dir(procs), 0755), IsNil) + c.Assert(os.WriteFile(procs, []byte(pids), 0644), IsNil) + } + } + + restore = cgroup.MockSyscallKill(func(pid int, sig syscall.Signal) error { + if pid == 1 || pid == 2 { + return fmt.Errorf("mock error for pid %d", pid) + } + s.ops = append(s.ops, fmt.Sprintf("kill-pid:%d, signal:%d", pid, sig)) + return nil + }) + defer restore() + + // Call failed and reported first error only + c.Assert(cgroup.KillSnapProcesses(context.TODO(), "foo"), ErrorMatches, "mock error for pid 1") + + // But kept going + if cgVersion == cgroup.V1 { + c.Assert(s.ops, DeepEquals, []string{ + "freeze-snap-processes-v1:foo", + "kill-pid:3, signal:9", + "kill-pid:4, signal:9", + "thaw-snap-processes-v1:foo", + }) + } else { + c.Assert(s.ops, DeepEquals, []string{ + // Kill first cgroup + "freeze-one-v2:snap.foo.app-1.1234-1234-1234.scope", + // Pid 1 not killed due to error + "thaw-one-v2:snap.foo.app-1.1234-1234-1234.scope", // Thaw on error + // Kill second cgroup + "freeze-one-v2:snap.foo.app-1.no-pids.scope", + // Pid 2 not killed due to error + "kill-pid:3, signal:9", + "thaw-one-v2:snap.foo.app-1.no-pids.scope", // Thaw on error + // Kill third cgroup + "freeze-one-v2:snap.foo.app-2.some-scope.scope", + "kill-pid:4, signal:9", + }) + } +} + +func (s *killSuite) TestKillSnapProcessesV1Error(c *C) { + const cgVersion = cgroup.V1 + s.testKillSnapProcessesError(c, cgVersion) +} + +func (s *killSuite) TestKillSnapProcessesV2Error(c *C) { + const cgVersion = cgroup.V2 + s.testKillSnapProcessesError(c, cgVersion) +}