From 9acfd7b1a30afeec48f1223870435b9f0d6a40c8 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Thu, 10 Aug 2023 18:32:24 +1000 Subject: [PATCH 1/2] timens: minor cleanups Fix up a few things that were flagged in the review of the original timens PR, namely around error handling and validation. Signed-off-by: Aleksa Sarai --- libcontainer/configs/validate/validator.go | 4 ++++ libcontainer/container_linux.go | 2 +- libcontainer/nsenter/nsexec.c | 14 ++++---------- libcontainer/specconv/spec_linux.go | 4 +--- 4 files changed, 10 insertions(+), 14 deletions(-) diff --git a/libcontainer/configs/validate/validator.go b/libcontainer/configs/validate/validator.go index 7e8e6c6f594..81eebba797f 100644 --- a/libcontainer/configs/validate/validator.go +++ b/libcontainer/configs/validate/validator.go @@ -120,6 +120,10 @@ func namespaces(config *configs.Config) error { if _, err := os.Stat("/proc/self/timens_offsets"); os.IsNotExist(err) { return errors.New("time namespaces aren't enabled in the kernel") } + } else { + if config.TimeOffsets != nil { + return errors.New("time namespace offsets specified, but time namespace isn't enabled in the config") + } } return nil diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index daec0ec4b71..1a2a6fe37ac 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -1192,7 +1192,7 @@ func (c *Container) bootstrapData(cloneFlags uintptr, nsMaps map[configs.Namespa } // write boottime and monotonic time ns offsets. - if c.config.Namespaces.Contains(configs.NEWTIME) && c.config.TimeOffsets != nil { + if c.config.TimeOffsets != nil { var offsetSpec bytes.Buffer for clock, offset := range c.config.TimeOffsets { fmt.Fprintf(&offsetSpec, "%s %d %d\n", clock, offset.Secs, offset.Nanosecs) diff --git a/libcontainer/nsenter/nsexec.c b/libcontainer/nsenter/nsexec.c index 8d4148cc645..07d9e0df130 100644 --- a/libcontainer/nsenter/nsexec.c +++ b/libcontainer/nsenter/nsexec.c @@ -758,15 +758,13 @@ void receive_idmapsources(int sockfd) receive_fd_sources(sockfd, "_LIBCONTAINER_IDMAP_FDS"); } -static void update_timens(char *map, size_t map_len) +static void update_timens_offsets(char *map, size_t map_len) { if (map == NULL || map_len == 0) return; write_log(DEBUG, "update /proc/self/timens_offsets to '%s'", map); - if (write_file(map, map_len, "/proc/self/timens_offsets") < 0) { - if (errno != EPERM) - bail("failed to update /proc/self/timens_offsets"); - } + if (write_file(map, map_len, "/proc/self/timens_offsets") < 0) + bail("failed to update /proc/self/timens_offsets"); } void nsexec(void) @@ -1174,6 +1172,7 @@ void nsexec(void) * was broken, so we'll just do it the long way anyway. */ try_unshare(config.cloneflags & ~CLONE_NEWCGROUP, "remaining namespaces (except cgroupns)"); + update_timens_offsets(config.timensoffset, config.timensoffset_len); /* Ask our parent to send the mount sources fds. */ if (config.mountsources) { @@ -1207,11 +1206,6 @@ void nsexec(void) bail("failed to sync with parent: SYNC_MOUNT_IDMAP_ACK: got %u", s); } - /* - * set boottime and monotonic timens offsets. - */ - update_timens(config.timensoffset, config.timensoffset_len); - /* * TODO: What about non-namespace clone flags that we're dropping here? * diff --git a/libcontainer/specconv/spec_linux.go b/libcontainer/specconv/spec_linux.go index 0afb4a6d8be..5ccaf356efb 100644 --- a/libcontainer/specconv/spec_linux.go +++ b/libcontainer/specconv/spec_linux.go @@ -422,6 +422,7 @@ func CreateLibcontainerConfig(opts *CreateOpts) (*configs.Config, error) { config.ReadonlyPaths = spec.Linux.ReadonlyPaths config.MountLabel = spec.Linux.MountLabel config.Sysctl = spec.Linux.Sysctl + config.TimeOffsets = spec.Linux.TimeOffsets if spec.Linux.Seccomp != nil { seccomp, err := SetupSeccomp(spec.Linux.Seccomp) if err != nil { @@ -436,9 +437,6 @@ func CreateLibcontainerConfig(opts *CreateOpts) (*configs.Config, error) { MemBwSchema: spec.Linux.IntelRdt.MemBwSchema, } } - - // update timens offsets - config.TimeOffsets = spec.Linux.TimeOffsets } // Set the host UID that should own the container's cgroup. From aa5f4c11372020343d0265ddad060c40963ac60a Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Thu, 10 Aug 2023 18:55:11 +1000 Subject: [PATCH 2/2] tests: add several timens tests These are not exhaustive, but at least confirm that the feature is not obviously broken (we correctly set the time offsets). Signed-off-by: Aleksa Sarai --- .../configs/validate/validator_test.go | 35 ++++++++++++ tests/integration/helpers.bash | 5 ++ tests/integration/timens.bats | 55 +++++++++++++++++++ 3 files changed, 95 insertions(+) create mode 100644 tests/integration/timens.bats diff --git a/libcontainer/configs/validate/validator_test.go b/libcontainer/configs/validate/validator_test.go index b6b2d372313..d2b3c70ad9d 100644 --- a/libcontainer/configs/validate/validator_test.go +++ b/libcontainer/configs/validate/validator_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/opencontainers/runc/libcontainer/configs" + "github.com/opencontainers/runtime-spec/specs-go" "golang.org/x/sys/unix" ) @@ -201,6 +202,40 @@ func TestValidateUsernamespaceWithoutUserNS(t *testing.T) { } } +func TestValidateTimeNamespace(t *testing.T) { + if _, err := os.Stat("/proc/self/ns/time"); os.IsNotExist(err) { + t.Skip("Test requires timens.") + } + config := &configs.Config{ + Rootfs: "/var", + Namespaces: configs.Namespaces( + []configs.Namespace{ + {Type: configs.NEWTIME}, + }, + ), + } + + err := Validate(config) + if err != nil { + t.Errorf("expected error to not occur %+v", err) + } +} + +func TestValidateTimeOffsetsWithoutTimeNamespace(t *testing.T) { + config := &configs.Config{ + Rootfs: "/var", + TimeOffsets: map[string]specs.LinuxTimeOffset{ + "boottime": {Secs: 150, Nanosecs: 314159}, + "monotonic": {Secs: 512, Nanosecs: 271818}, + }, + } + + err := Validate(config) + if err == nil { + t.Error("Expected error to occur but it was nil") + } +} + // TestConvertSysctlVariableToDotsSeparator tests whether the sysctl variable // can be correctly converted to a dot as a separator. func TestConvertSysctlVariableToDotsSeparator(t *testing.T) { diff --git a/tests/integration/helpers.bash b/tests/integration/helpers.bash index e1ce4214418..65912951d58 100755 --- a/tests/integration/helpers.bash +++ b/tests/integration/helpers.bash @@ -429,6 +429,11 @@ function requires() { skip_me=1 fi ;; + timens) + if [ ! -e "/proc/self/ns/time" ]; then + skip_me=1 + fi + ;; cgroups_v1) init_cgroup_paths if [ ! -v CGROUP_V1 ]; then diff --git a/tests/integration/timens.bats b/tests/integration/timens.bats new file mode 100644 index 00000000000..e0d21d5510f --- /dev/null +++ b/tests/integration/timens.bats @@ -0,0 +1,55 @@ +#!/usr/bin/env bats + +load helpers + +function setup() { + setup_busybox +} + +function teardown() { + teardown_bundle +} + +@test "runc run [timens offsets with no timens]" { + requires timens + + update_config '.process.args = ["cat", "/proc/self/timens_offsets"]' + update_config '.linux.namespaces = .linux.namespace | map(select(.type != "time"))' + update_config '.linux.timeOffsets = { + "monotonic": { "secs": 7881, "nanosecs": 2718281 }, + "boottime": { "secs": 1337, "nanosecs": 3141519 } + }' + + runc run test_busybox + [ "$status" -ne 0 ] +} + +@test "runc run [timens with no offsets]" { + requires timens + + update_config '.process.args = ["cat", "/proc/self/timens_offsets"]' + update_config '.linux.namespaces += [{"type": "time"}] + | .linux.timeOffsets = null' + + runc run test_busybox + [ "$status" -eq 0 ] + # Default offsets are 0. + grep -E '^monotonic\s+0\s+0$' <<<"$output" + grep -E '^boottime\s+0\s+0$' <<<"$output" +} + +@test "runc run [simple timens]" { + requires timens + + update_config '.process.args = ["cat", "/proc/self/timens_offsets"]' + update_config '.linux.namespaces += [{"type": "time"}] + | .linux.timeOffsets = { + "monotonic": { "secs": 7881, "nanosecs": 2718281 }, + "boottime": { "secs": 1337, "nanosecs": 3141519 } + }' + + runc run test_busybox + [ "$status" -eq 0 ] + grep -E '^monotonic\s+7881\s+2718281$' <<<"$output" + grep -E '^boottime\s+1337\s+3141519$' <<<"$output" +}