From f0b1bd434eb00a0f13cbbc141078a95ba0bc15a4 Mon Sep 17 00:00:00 2001 From: Jamie Liu Date: Thu, 17 Sep 2020 11:50:59 -0700 Subject: Deflake vdso_clock_gettime test. PiperOrigin-RevId: 332281930 --- test/syscalls/linux/vdso_clock_gettime.cc | 64 ++++++++----------------------- 1 file changed, 17 insertions(+), 47 deletions(-) diff --git a/test/syscalls/linux/vdso_clock_gettime.cc b/test/syscalls/linux/vdso_clock_gettime.cc index ce1899f45..52b67249d 100644 --- a/test/syscalls/linux/vdso_clock_gettime.cc +++ b/test/syscalls/linux/vdso_clock_gettime.cc @@ -38,8 +38,6 @@ std::string PrintClockId(::testing::TestParamInfo info) { switch (info.param) { case CLOCK_MONOTONIC: return "CLOCK_MONOTONIC"; - case CLOCK_REALTIME: - return "CLOCK_REALTIME"; case CLOCK_BOOTTIME: return "CLOCK_BOOTTIME"; default: @@ -47,59 +45,31 @@ std::string PrintClockId(::testing::TestParamInfo info) { } } -class CorrectVDSOClockTest : public ::testing::TestWithParam {}; +class MonotonicVDSOClockTest : public ::testing::TestWithParam {}; -TEST_P(CorrectVDSOClockTest, IsCorrect) { +TEST_P(MonotonicVDSOClockTest, IsCorrect) { + // Check that when we alternate readings from the clock_gettime syscall and + // the VDSO's implementation, we observe the combined sequence as being + // monotonic. struct timespec tvdso, tsys; absl::Time vdso_time, sys_time; - uint64_t total_calls = 0; - - // It is expected that 82.5% of clock_gettime calls will be less than 100us - // skewed from the system time. - // Unfortunately this is not only influenced by the VDSO clock skew, but also - // by arbitrary scheduling delays and the like. The test is therefore - // regularly disabled. - std::map> confidence = - { - {absl::Microseconds(100), std::make_tuple(0.825, 0, 0)}, - {absl::Microseconds(250), std::make_tuple(0.94, 0, 0)}, - {absl::Milliseconds(1), std::make_tuple(0.999, 0, 0)}, - }; - - absl::Time start = absl::Now(); - while (absl::Now() < start + absl::Seconds(30)) { - EXPECT_THAT(clock_gettime(GetParam(), &tvdso), SyscallSucceeds()); - EXPECT_THAT(syscall(__NR_clock_gettime, GetParam(), &tsys), - SyscallSucceeds()); - + ASSERT_THAT(syscall(__NR_clock_gettime, GetParam(), &tsys), + SyscallSucceeds()); + sys_time = absl::TimeFromTimespec(tsys); + auto end = absl::Now() + absl::Seconds(10); + while (absl::Now() < end) { + ASSERT_THAT(clock_gettime(GetParam(), &tvdso), SyscallSucceeds()); vdso_time = absl::TimeFromTimespec(tvdso); - - for (auto const& conf : confidence) { - std::get<1>(confidence[conf.first]) += - (sys_time - vdso_time) < conf.first; - } - + EXPECT_LE(sys_time, vdso_time); + ASSERT_THAT(syscall(__NR_clock_gettime, GetParam(), &tsys), + SyscallSucceeds()); sys_time = absl::TimeFromTimespec(tsys); - - for (auto const& conf : confidence) { - std::get<2>(confidence[conf.first]) += - (vdso_time - sys_time) < conf.first; - } - - ++total_calls; - } - - for (auto const& conf : confidence) { - EXPECT_GE(std::get<1>(conf.second) / static_cast(total_calls), - std::get<0>(conf.second)); - EXPECT_GE(std::get<2>(conf.second) / static_cast(total_calls), - std::get<0>(conf.second)); + EXPECT_LE(vdso_time, sys_time); } } -INSTANTIATE_TEST_SUITE_P(ClockGettime, CorrectVDSOClockTest, - ::testing::Values(CLOCK_MONOTONIC, CLOCK_REALTIME, - CLOCK_BOOTTIME), +INSTANTIATE_TEST_SUITE_P(ClockGettime, MonotonicVDSOClockTest, + ::testing::Values(CLOCK_MONOTONIC, CLOCK_BOOTTIME), PrintClockId); } // namespace -- cgit v1.2.3 From da07e38f7c986dca568466beea8a0b7db60f09db Mon Sep 17 00:00:00 2001 From: Fabricio Voznika Date: Thu, 17 Sep 2020 11:55:54 -0700 Subject: Remove option to panic gofer Gofer panics are suppressed by p9 server and an error is returned to the caller, making it effectively the same as returning EROFS. PiperOrigin-RevId: 332282959 --- runsc/cmd/gofer.go | 15 ++++++--------- runsc/container/container.go | 3 --- runsc/fsgofer/fsgofer.go | 3 --- runsc/fsgofer/fsgofer_test.go | 23 ----------------------- 4 files changed, 6 insertions(+), 38 deletions(-) diff --git a/runsc/cmd/gofer.go b/runsc/cmd/gofer.go index bba00d551..371fcc0ae 100644 --- a/runsc/cmd/gofer.go +++ b/runsc/cmd/gofer.go @@ -62,9 +62,8 @@ type Gofer struct { applyCaps bool setUpRoot bool - panicOnWrite bool - specFD int - mountsFD int + specFD int + mountsFD int } // Name implements subcommands.Command. @@ -87,7 +86,6 @@ func (g *Gofer) SetFlags(f *flag.FlagSet) { f.StringVar(&g.bundleDir, "bundle", "", "path to the root of the bundle directory, defaults to the current directory") f.Var(&g.ioFDs, "io-fds", "list of FDs to connect 9P servers. They must follow this order: root first, then mounts as defined in the spec") f.BoolVar(&g.applyCaps, "apply-caps", true, "if true, apply capabilities to restrict what the Gofer process can do") - f.BoolVar(&g.panicOnWrite, "panic-on-write", false, "if true, panics on attempts to write to RO mounts. RW mounts are unnaffected") f.BoolVar(&g.setUpRoot, "setup-root", true, "if true, set up an empty root for the process") f.IntVar(&g.specFD, "spec-fd", -1, "required fd with the container spec") f.IntVar(&g.mountsFD, "mounts-fd", -1, "mountsFD is the file descriptor to write list of mounts after they have been resolved (direct paths, no symlinks).") @@ -168,8 +166,7 @@ func (g *Gofer) Execute(_ context.Context, f *flag.FlagSet, args ...interface{}) // Start with root mount, then add any other additional mount as needed. ats := make([]p9.Attacher, 0, len(spec.Mounts)+1) ap, err := fsgofer.NewAttachPoint("/", fsgofer.Config{ - ROMount: spec.Root.Readonly || conf.Overlay, - PanicOnWrite: g.panicOnWrite, + ROMount: spec.Root.Readonly || conf.Overlay, }) if err != nil { Fatalf("creating attach point: %v", err) @@ -181,9 +178,8 @@ func (g *Gofer) Execute(_ context.Context, f *flag.FlagSet, args ...interface{}) for _, m := range spec.Mounts { if specutils.Is9PMount(m) { cfg := fsgofer.Config{ - ROMount: isReadonlyMount(m.Options) || conf.Overlay, - PanicOnWrite: g.panicOnWrite, - HostUDS: conf.FSGoferHostUDS, + ROMount: isReadonlyMount(m.Options) || conf.Overlay, + HostUDS: conf.FSGoferHostUDS, } ap, err := fsgofer.NewAttachPoint(m.Destination, cfg) if err != nil { @@ -316,6 +312,7 @@ func setupRootFS(spec *specs.Spec, conf *config.Config) error { if err != nil { return fmt.Errorf("resolving symlinks to %q: %v", spec.Process.Cwd, err) } + log.Infof("Create working directory %q if needed", spec.Process.Cwd) if err := os.MkdirAll(dst, 0755); err != nil { return fmt.Errorf("creating working directory %q: %v", spec.Process.Cwd, err) } diff --git a/runsc/container/container.go b/runsc/container/container.go index 6e1d6a568..63478ba8c 100644 --- a/runsc/container/container.go +++ b/runsc/container/container.go @@ -902,9 +902,6 @@ func (c *Container) createGoferProcess(spec *specs.Spec, conf *config.Config, bu } args = append(args, "gofer", "--bundle", bundleDir) - if conf.Overlay { - args = append(args, "--panic-on-write=true") - } // Open the spec file to donate to the sandbox. specFile, err := specutils.OpenSpec(bundleDir) diff --git a/runsc/fsgofer/fsgofer.go b/runsc/fsgofer/fsgofer.go index 4268d97a1..0b628c8ce 100644 --- a/runsc/fsgofer/fsgofer.go +++ b/runsc/fsgofer/fsgofer.go @@ -1181,9 +1181,6 @@ func extractErrno(err error) unix.Errno { func (l *localFile) checkROMount() error { if conf := l.attachPoint.conf; conf.ROMount { - if conf.PanicOnWrite { - panic("attempt to write to RO mount") - } return unix.EROFS } return nil diff --git a/runsc/fsgofer/fsgofer_test.go b/runsc/fsgofer/fsgofer_test.go index 0e4945b3d..a84206686 100644 --- a/runsc/fsgofer/fsgofer_test.go +++ b/runsc/fsgofer/fsgofer_test.go @@ -553,29 +553,6 @@ func TestROMountChecks(t *testing.T) { }) } -func TestROMountPanics(t *testing.T) { - conf := Config{ROMount: true, PanicOnWrite: true} - uid := p9.UID(os.Getuid()) - gid := p9.GID(os.Getgid()) - - runCustom(t, allTypes, []Config{conf}, func(t *testing.T, s state) { - if s.fileType != unix.S_IFLNK { - assertPanic(t, func() { s.file.Open(p9.WriteOnly) }) - } - assertPanic(t, func() { s.file.Create("some_file", p9.ReadWrite, 0777, uid, gid) }) - assertPanic(t, func() { s.file.Mkdir("some_dir", 0777, uid, gid) }) - assertPanic(t, func() { s.file.RenameAt("some_file", s.file, "other_file") }) - assertPanic(t, func() { s.file.Symlink("some_place", "some_symlink", uid, gid) }) - assertPanic(t, func() { s.file.UnlinkAt("some_file", 0) }) - assertPanic(t, func() { s.file.Link(s.file, "some_link") }) - assertPanic(t, func() { s.file.Mknod("some-nod", 0777, 1, 2, uid, gid) }) - - valid := p9.SetAttrMask{Size: true} - attr := p9.SetAttr{Size: 0} - assertPanic(t, func() { s.file.SetAttr(valid, attr) }) - }) -} - func TestWalkNotFound(t *testing.T) { runCustom(t, []uint32{unix.S_IFDIR}, allConfs, func(t *testing.T, s state) { if _, _, err := s.file.Walk([]string{"nobody-here"}); err != unix.ENOENT { -- cgit v1.2.3