From 3688e6e99d16b0c6ecb7c8b3528a541ce6afe3a7 Mon Sep 17 00:00:00 2001 From: Adrien Leravat Date: Fri, 21 Jun 2019 22:40:16 -0700 Subject: [PATCH 1/3] Add CLOCK_BOOTTIME as a CLOCK_MONOTONIC alias Makes CLOCK_BOOTTIME available with * clock_gettime * timerfd_create * clock_gettime vDSO CLOCK_BOOTTIME is implemented as an alias to CLOCK_MONOTONIC. CLOCK_MONOTONIC already keeps track of time across save and restore. This is the closest possible behavior to Linux CLOCK_BOOTIME, as there is no concept of suspend/resume. Updates google/gvisor#218 --- pkg/sentry/syscalls/linux/sys_time.go | 7 ++++++- pkg/sentry/syscalls/linux/sys_timerfd.go | 2 +- test/syscalls/linux/clock_gettime.cc | 8 +++++--- test/syscalls/linux/vdso_clock_gettime.cc | 6 +++++- vdso/vdso.cc | 2 ++ 5 files changed, 19 insertions(+), 6 deletions(-) diff --git a/pkg/sentry/syscalls/linux/sys_time.go b/pkg/sentry/syscalls/linux/sys_time.go index fe8725191..0f36e4cd0 100644 --- a/pkg/sentry/syscalls/linux/sys_time.go +++ b/pkg/sentry/syscalls/linux/sys_time.go @@ -121,8 +121,13 @@ func getClock(t *kernel.Task, clockID int32) (ktime.Clock, error) { switch clockID { case linux.CLOCK_REALTIME, linux.CLOCK_REALTIME_COARSE: return t.Kernel().RealtimeClock(), nil - case linux.CLOCK_MONOTONIC, linux.CLOCK_MONOTONIC_COARSE, linux.CLOCK_MONOTONIC_RAW: + case linux.CLOCK_MONOTONIC, linux.CLOCK_MONOTONIC_COARSE, + linux.CLOCK_MONOTONIC_RAW, linux.CLOCK_BOOTTIME: // CLOCK_MONOTONIC approximates CLOCK_MONOTONIC_RAW. + // CLOCK_BOOTTIME is internally mapped to CLOCK_MONOTONIC, as: + // - CLOCK_BOOTTIME should behave as CLOCK_MONOTONIC while also including suspend time. + // - gVisor has no concept of suspend/resume. + // - CLOCK_MONOTONIC already includes save/restore time, which is the closest to suspend time. return t.Kernel().MonotonicClock(), nil case linux.CLOCK_PROCESS_CPUTIME_ID: return t.ThreadGroup().CPUClock(), nil diff --git a/pkg/sentry/syscalls/linux/sys_timerfd.go b/pkg/sentry/syscalls/linux/sys_timerfd.go index ea6d44315..b889a2a93 100644 --- a/pkg/sentry/syscalls/linux/sys_timerfd.go +++ b/pkg/sentry/syscalls/linux/sys_timerfd.go @@ -38,7 +38,7 @@ func TimerfdCreate(t *kernel.Task, args arch.SyscallArguments) (uintptr, *kernel switch clockID { case linux.CLOCK_REALTIME: c = t.Kernel().RealtimeClock() - case linux.CLOCK_MONOTONIC: + case linux.CLOCK_MONOTONIC, linux.CLOCK_BOOTTIME: c = t.Kernel().MonotonicClock() default: return 0, nil, syserror.EINVAL diff --git a/test/syscalls/linux/clock_gettime.cc b/test/syscalls/linux/clock_gettime.cc index 335a38d41..9efd20b1a 100644 --- a/test/syscalls/linux/clock_gettime.cc +++ b/test/syscalls/linux/clock_gettime.cc @@ -132,6 +132,9 @@ std::string PrintClockId(::testing::TestParamInfo info) { return "CLOCK_MONOTONIC_COARSE"; case CLOCK_MONOTONIC_RAW: return "CLOCK_MONOTONIC_RAW"; + case CLOCK_BOOTTIME: + // CLOCK_BOOTTIME is a monotonic clock. + return "CLOCK_BOOTTIME"; default: return absl::StrCat(info.param); } @@ -140,15 +143,14 @@ std::string PrintClockId(::testing::TestParamInfo info) { INSTANTIATE_TEST_SUITE_P(ClockGettime, MonotonicClockTest, ::testing::Values(CLOCK_MONOTONIC, CLOCK_MONOTONIC_COARSE, - CLOCK_MONOTONIC_RAW), + CLOCK_MONOTONIC_RAW, + CLOCK_BOOTTIME), PrintClockId); TEST(ClockGettime, UnimplementedReturnsEINVAL) { SKIP_IF(!IsRunningOnGvisor()); struct timespec tp; - EXPECT_THAT(clock_gettime(CLOCK_BOOTTIME, &tp), - SyscallFailsWithErrno(EINVAL)); EXPECT_THAT(clock_gettime(CLOCK_REALTIME_ALARM, &tp), SyscallFailsWithErrno(EINVAL)); EXPECT_THAT(clock_gettime(CLOCK_BOOTTIME_ALARM, &tp), diff --git a/test/syscalls/linux/vdso_clock_gettime.cc b/test/syscalls/linux/vdso_clock_gettime.cc index 759a50569..7e56c3de0 100644 --- a/test/syscalls/linux/vdso_clock_gettime.cc +++ b/test/syscalls/linux/vdso_clock_gettime.cc @@ -39,6 +39,8 @@ std::string PrintClockId(::testing::TestParamInfo info) { return "CLOCK_MONOTONIC"; case CLOCK_REALTIME: return "CLOCK_REALTIME"; + case CLOCK_BOOTTIME: + return "CLOCK_BOOTTIME"; default: return absl::StrCat(info.param); } @@ -95,7 +97,9 @@ TEST_P(CorrectVDSOClockTest, IsCorrect) { } INSTANTIATE_TEST_SUITE_P(ClockGettime, CorrectVDSOClockTest, - ::testing::Values(CLOCK_MONOTONIC, CLOCK_REALTIME), + ::testing::Values(CLOCK_MONOTONIC, + CLOCK_REALTIME, + CLOCK_BOOTTIME), PrintClockId); } // namespace diff --git a/vdso/vdso.cc b/vdso/vdso.cc index 6265ad217..0bded13ac 100644 --- a/vdso/vdso.cc +++ b/vdso/vdso.cc @@ -33,6 +33,8 @@ int __common_clock_gettime(clockid_t clock, struct timespec* ts) { ret = ClockRealtime(ts); break; + // Fallthrough, CLOCK_BOOTTIME is an alias for CLOCK_MONOTONIC + case CLOCK_BOOTTIME: case CLOCK_MONOTONIC: ret = ClockMonotonic(ts); break; From 02d1bd67f073dd8e99ce591d1285d1bbc152b424 Mon Sep 17 00:00:00 2001 From: Adrien Leravat Date: Tue, 16 Jul 2019 14:03:40 -0700 Subject: [PATCH 2/3] Add CLOCK_BOOTTIME tests to timerfd.cc --- test/syscalls/linux/timerfd.cc | 77 +++++++++++++++++++++------------- 1 file changed, 48 insertions(+), 29 deletions(-) diff --git a/test/syscalls/linux/timerfd.cc b/test/syscalls/linux/timerfd.cc index 9df53612f..bbdf76e7a 100644 --- a/test/syscalls/linux/timerfd.cc +++ b/test/syscalls/linux/timerfd.cc @@ -44,21 +44,24 @@ PosixErrorOr TimerfdCreate(int clockid, int flags) { // // - Because clock_gettime(CLOCK_MONOTONIC) is implemented through the VDSO, // it technically uses a closely-related, but distinct, time domain from the -// CLOCK_MONOTONIC used to trigger timerfd expirations. +// CLOCK_MONOTONIC used to trigger timerfd expirations. The same applies to +// CLOCK_BOOTTIME which is an alias for CLOCK_MONOTONIC. absl::Duration TimerSlack() { return absl::Milliseconds(500); } -TEST(TimerfdTest, IsInitiallyStopped) { - auto const tfd = ASSERT_NO_ERRNO_AND_VALUE(TimerfdCreate(CLOCK_MONOTONIC, 0)); +class TimerfdTest : public ::testing::TestWithParam {}; + +TEST_P(TimerfdTest, IsInitiallyStopped) { + auto const tfd = ASSERT_NO_ERRNO_AND_VALUE(TimerfdCreate(GetParam(), 0)); struct itimerspec its = {}; ASSERT_THAT(timerfd_gettime(tfd.get(), &its), SyscallSucceeds()); EXPECT_EQ(0, its.it_value.tv_sec); EXPECT_EQ(0, its.it_value.tv_nsec); } -TEST(TimerfdTest, SingleShot) { +TEST_P(TimerfdTest, SingleShot) { constexpr absl::Duration kDelay = absl::Seconds(1); - auto const tfd = ASSERT_NO_ERRNO_AND_VALUE(TimerfdCreate(CLOCK_MONOTONIC, 0)); + auto const tfd = ASSERT_NO_ERRNO_AND_VALUE(TimerfdCreate(GetParam(), 0)); struct itimerspec its = {}; its.it_value = absl::ToTimespec(kDelay); ASSERT_THAT(timerfd_settime(tfd.get(), /* flags = */ 0, &its, nullptr), @@ -72,11 +75,11 @@ TEST(TimerfdTest, SingleShot) { EXPECT_EQ(1, val); } -TEST(TimerfdTest, Periodic) { +TEST_P(TimerfdTest, Periodic) { constexpr absl::Duration kDelay = absl::Seconds(1); constexpr int kPeriods = 3; - auto const tfd = ASSERT_NO_ERRNO_AND_VALUE(TimerfdCreate(CLOCK_MONOTONIC, 0)); + auto const tfd = ASSERT_NO_ERRNO_AND_VALUE(TimerfdCreate(GetParam(), 0)); struct itimerspec its = {}; its.it_value = absl::ToTimespec(kDelay); its.it_interval = absl::ToTimespec(kDelay); @@ -92,10 +95,10 @@ TEST(TimerfdTest, Periodic) { EXPECT_GE(val, kPeriods); } -TEST(TimerfdTest, BlockingRead) { +TEST_P(TimerfdTest, BlockingRead) { constexpr absl::Duration kDelay = absl::Seconds(3); - auto const tfd = ASSERT_NO_ERRNO_AND_VALUE(TimerfdCreate(CLOCK_MONOTONIC, 0)); + auto const tfd = ASSERT_NO_ERRNO_AND_VALUE(TimerfdCreate(GetParam(), 0)); struct itimerspec its = {}; its.it_value.tv_sec = absl::ToInt64Seconds(kDelay); auto const start_time = absl::Now(); @@ -111,11 +114,11 @@ TEST(TimerfdTest, BlockingRead) { EXPECT_GE((end_time - start_time) + TimerSlack(), kDelay); } -TEST(TimerfdTest, NonblockingRead_NoRandomSave) { +TEST_P(TimerfdTest, NonblockingRead_NoRandomSave) { constexpr absl::Duration kDelay = absl::Seconds(5); auto const tfd = - ASSERT_NO_ERRNO_AND_VALUE(TimerfdCreate(CLOCK_MONOTONIC, TFD_NONBLOCK)); + ASSERT_NO_ERRNO_AND_VALUE(TimerfdCreate(GetParam(), TFD_NONBLOCK)); // Since the timer is initially disabled and has never fired, read should // return EAGAIN. @@ -148,11 +151,11 @@ TEST(TimerfdTest, NonblockingRead_NoRandomSave) { SyscallFailsWithErrno(EAGAIN)); } -TEST(TimerfdTest, BlockingPoll_SetTimeResetsExpirations) { +TEST_P(TimerfdTest, BlockingPoll_SetTimeResetsExpirations) { constexpr absl::Duration kDelay = absl::Seconds(3); auto const tfd = - ASSERT_NO_ERRNO_AND_VALUE(TimerfdCreate(CLOCK_MONOTONIC, TFD_NONBLOCK)); + ASSERT_NO_ERRNO_AND_VALUE(TimerfdCreate(GetParam(), TFD_NONBLOCK)); struct itimerspec its = {}; its.it_value.tv_sec = absl::ToInt64Seconds(kDelay); auto const start_time = absl::Now(); @@ -181,15 +184,15 @@ TEST(TimerfdTest, BlockingPoll_SetTimeResetsExpirations) { SyscallFailsWithErrno(EAGAIN)); } -TEST(TimerfdTest, SetAbsoluteTime) { +TEST_P(TimerfdTest, SetAbsoluteTime) { constexpr absl::Duration kDelay = absl::Seconds(3); // Use a non-blocking timerfd so that if TFD_TIMER_ABSTIME is incorrectly // non-functional, we get EAGAIN rather than a test timeout. auto const tfd = - ASSERT_NO_ERRNO_AND_VALUE(TimerfdCreate(CLOCK_MONOTONIC, TFD_NONBLOCK)); + ASSERT_NO_ERRNO_AND_VALUE(TimerfdCreate(GetParam(), TFD_NONBLOCK)); struct itimerspec its = {}; - ASSERT_THAT(clock_gettime(CLOCK_MONOTONIC, &its.it_value), SyscallSucceeds()); + ASSERT_THAT(clock_gettime(GetParam(), &its.it_value), SyscallSucceeds()); its.it_value.tv_sec += absl::ToInt64Seconds(kDelay); ASSERT_THAT(timerfd_settime(tfd.get(), TFD_TIMER_ABSTIME, &its, nullptr), SyscallSucceeds()); @@ -201,7 +204,35 @@ TEST(TimerfdTest, SetAbsoluteTime) { EXPECT_EQ(1, val); } -TEST(TimerfdTest, ClockRealtime) { +TEST_P(TimerfdTest, IllegalReadWrite) { + auto const tfd = + ASSERT_NO_ERRNO_AND_VALUE(TimerfdCreate(GetParam(), TFD_NONBLOCK)); + uint64_t val = 0; + EXPECT_THAT(PreadFd(tfd.get(), &val, sizeof(val), 0), + SyscallFailsWithErrno(ESPIPE)); + EXPECT_THAT(WriteFd(tfd.get(), &val, sizeof(val)), + SyscallFailsWithErrno(EINVAL)); + EXPECT_THAT(PwriteFd(tfd.get(), &val, sizeof(val), 0), + SyscallFailsWithErrno(ESPIPE)); +} + +std::string PrintClockId(::testing::TestParamInfo info) { + switch (info.param) { + case CLOCK_MONOTONIC: + return "CLOCK_MONOTONIC"; + case CLOCK_BOOTTIME: + return "CLOCK_BOOTTIME"; + default: + return absl::StrCat(info.param); + } +} + +INSTANTIATE_TEST_SUITE_P(AllTimerTypes, + TimerfdTest, + ::testing::Values(CLOCK_MONOTONIC, CLOCK_BOOTTIME), + PrintClockId); + +TEST(TimerfdClockRealtimeTest, ClockRealtime) { // Since CLOCK_REALTIME can, by definition, change, we can't make any // non-flaky assertions about the amount of time it takes for a // CLOCK_REALTIME-based timer to expire. Just check that it expires at all, @@ -220,18 +251,6 @@ TEST(TimerfdTest, ClockRealtime) { EXPECT_EQ(1, val); } -TEST(TimerfdTest, IllegalReadWrite) { - auto const tfd = - ASSERT_NO_ERRNO_AND_VALUE(TimerfdCreate(CLOCK_MONOTONIC, TFD_NONBLOCK)); - uint64_t val = 0; - EXPECT_THAT(PreadFd(tfd.get(), &val, sizeof(val), 0), - SyscallFailsWithErrno(ESPIPE)); - EXPECT_THAT(WriteFd(tfd.get(), &val, sizeof(val)), - SyscallFailsWithErrno(EINVAL)); - EXPECT_THAT(PwriteFd(tfd.get(), &val, sizeof(val), 0), - SyscallFailsWithErrno(ESPIPE)); -} - } // namespace } // namespace testing From 2d11fa05f7b705f74c737f5a59fe40414bb6f8d8 Mon Sep 17 00:00:00 2001 From: Adrien Leravat Date: Wed, 17 Jul 2019 20:25:18 -0700 Subject: [PATCH 3/3] sys_time: Wrap comments to 80 columns --- pkg/sentry/syscalls/linux/sys_time.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/sentry/syscalls/linux/sys_time.go b/pkg/sentry/syscalls/linux/sys_time.go index 0f36e4cd0..4b3f043a2 100644 --- a/pkg/sentry/syscalls/linux/sys_time.go +++ b/pkg/sentry/syscalls/linux/sys_time.go @@ -125,9 +125,11 @@ func getClock(t *kernel.Task, clockID int32) (ktime.Clock, error) { linux.CLOCK_MONOTONIC_RAW, linux.CLOCK_BOOTTIME: // CLOCK_MONOTONIC approximates CLOCK_MONOTONIC_RAW. // CLOCK_BOOTTIME is internally mapped to CLOCK_MONOTONIC, as: - // - CLOCK_BOOTTIME should behave as CLOCK_MONOTONIC while also including suspend time. + // - CLOCK_BOOTTIME should behave as CLOCK_MONOTONIC while also + // including suspend time. // - gVisor has no concept of suspend/resume. - // - CLOCK_MONOTONIC already includes save/restore time, which is the closest to suspend time. + // - CLOCK_MONOTONIC already includes save/restore time, which is + // the closest to suspend time. return t.Kernel().MonotonicClock(), nil case linux.CLOCK_PROCESS_CPUTIME_ID: return t.ThreadGroup().CPUClock(), nil