Support time namespace #3876

chethanah · 2023-05-22T17:55:36Z

closes Support time namespaces #2345

Background

runtime-spec now supports time namespace
- Add support for time namespace runtime-spec#1151 - Merged
Existing issue in runc - Support time namespaces #2345

Support time ns in runc

This PR supports time ns by updating the monotonic and boottime clock offest in /proc/self/timens_offsets
- Support BootTimeNsAttr and MonotonicNsAttr netlink message type to send to bootstrap program
- Update time ns in STAGE_CHILD after unsharing all the clone flags (stage-1)
- validate path /proc/self/timens_offsets to validate if time ns enabled
Add test cases
- nsenter_test.go only checks for invalid ns path and type for netlink requests. No specific _test.go for time ns is required.
- Add timens.bats integration test? - TBD
❌ Update documentations - Not required
- runtime-spec documentations are already updated. No docs required in runc.
- https://github.com/opencontainers/runtime-spec/blob/main/config-linux.md#namespaces
- https://github.com/opencontainers/runtime-spec/blob/main/config-linux.md#offset-for-time-namespace

Unit test

Launch container with boottime offsets

# jq '.linux.timeOffsets["boottime"]' config.json 
{
  "secs": 999999999,
  "nanosecs": 0
}

# jq '.linux.namespaces[5]' config.json 
{
  "type": "time"
}

# jq '.process.args' config.json 
[
  "uptime",
  "-s"
]

# uptime -s; ./runc run test 
2023-05-22 17:05:57
1991-09-13 09:49:18

Discussion points

The time offset is updated to /proc/self/timens_offsets
- is it important to set stage1_pid/stage2_pid like done in update_uidmap?
Is it advisable to set offsets elsewhere apart from STAGE_CHILD?
- Ref: https://man7.org/linux/man-pages/man7/time_namespaces.7.html
- but after the first process has been created in or has entered the namespace, write(2)s on this file fail with the error EACCES.
- Therefore, considered to write during STAGE_CHILD after unshared ns

chethanah · 2023-05-22T17:56:00Z

CC: @KentaTada
Create PR for supporting time ns in runc

chethanah · 2023-05-22T17:59:02Z

Kindly review and suggest for changes or additional discussion points.
Test cases and documentation will be added if core implementation is finalized

chethanah · 2023-05-23T06:18:54Z

CC: @AkihiroSuda

AkihiroSuda · 2023-06-10T07:47:04Z

CI is failing

libcontainer/specconv/spec_linux.go

libcontainer/container_linux.go

chethanah · 2023-07-11T12:36:28Z

The CI is now passing after removing logrus print statement.

chethanah · 2023-07-12T02:02:48Z

Additional merge commit added while rebasing changes from main. I'll update my branch by removing the merge commit.

libcontainer/container_linux.go

libcontainer/specconv/spec_linux.go

cyphar

Sorry for not looking at this earlier, only one somewhat significant thing needs to be changed. And this unfortunately needs a rebase due to the just-merged id-mapped-mounts work.

This could also really use some integration tests. If you're not comfortable setting those up, I can do them in a follow-up PR.

cyphar · 2023-07-20T00:36:41Z

libcontainer/configs/config.go

@@ -212,6 +212,9 @@ type Config struct {
 	// RootlessCgroups is set when unlikely to have the full access to cgroups.
 	// When RootlessCgroups is set, cgroups errors are ignored.
 	RootlessCgroups bool `json:"rootless_cgroups,omitempty"`
+
+	// TimeOffsets specifies the offset for supporting time namespaces.
+	TimeOffsets map[string]specs.LinuxTimeOffset `json:"timeOffsets,omitempty"`


To match the rest of the state json:

Suggested change

TimeOffsets map[string]specs.LinuxTimeOffset `json:"timeOffsets,omitempty"`

TimeOffsets map[string]specs.LinuxTimeOffset `json:"time_offsets,omitempty"`

cyphar · 2023-07-20T00:41:36Z

libcontainer/container_linux.go

+	// write boottime and monotonic offsets
+	// 0 is default value in /proc/PID/timens_offsets
+	// if 0, do not bootstrap data
+	if c.config.TimeOffsets != nil {
+
+		if c.config.TimeOffsets["boottime"].Nanosecs != 0 || c.config.TimeOffsets["boottime"].Secs != 0 {
+			b, err := encodeTimeNs(c.config.TimeOffsets["boottime"])
+			if err != nil {
+				return nil, err
+			}
+			r.AddData(&Bytemsg{
+				Type:  BootTimeNsAttr,
+				Value: append([]byte("boottime "), b...),
+			})
+		}
+
+		if c.config.TimeOffsets["monotonic"].Nanosecs != 0 || c.config.TimeOffsets["monotonic"].Secs != 0 {
+			b, err := encodeTimeNs(c.config.TimeOffsets["monotonic"])
+			if err != nil {
+				return nil, err
+			}
+			r.AddData(&Bytemsg{
+				Type:  MonotonicNsAttr,
+				Value: append([]byte("monotonic "), b...),
+			})
+		}
+	}
+


These can be written with one string by separating them with newlines, so there's no need to add multiple netlink entries for this. Just generate one string into a TimeOffsetsAttr and write it in one shot in nsexec.c.

cyphar · 2023-07-20T00:53:51Z

libcontainer/nsenter/nsexec.c

+			 * Update timens offsets
+			 * set boottime and monotonic offsets
+			 */
+			write_log(DEBUG, "set timens offsets %s", config.boottime);


You're outputting this debug information twice (both here and in update_timens). You can remove this one.

kolyshkin · 2023-07-28T23:09:20Z

libcontainer/container_linux.go

+		if c.config.TimeOffsets["boottime"].Nanosecs != 0 || c.config.TimeOffsets["boottime"].Secs != 0 {
+			b, err := encodeTimeNs(c.config.TimeOffsets["boottime"])


You use 3 map lookups here, while 1 should be enough. Make encodeTimeNs check if the fields are all 0 and return empty string in such case, add a check for empty string below.

It also looks like this would be a good place to write something like

for k, v := c.Config.TimeOffsets { ... }

making the code more generic and avoiding to use specific strings.

kolyshkin · 2023-07-28T23:10:05Z

libcontainer/nsenter/nsexec.c

@@ -95,6 +95,13 @@ struct nlconfig_t {
 	/* Mount sources opened outside the container userns. */
 	char *mountsources;
 	size_t mountsources_len;
+
+	/* Time NS settings for boottime and monotonic */


nit: missing period.

kolyshkin · 2023-07-28T23:11:03Z

libcontainer/nsenter/nsexec.c

+			 * Update timens offsets
+			 * set boottime and monotonic offsets


nit: missing periods. Please write complete sentences in comments, otherwise it's harder to read (and maintain).

kolyshkin · 2023-07-28T23:12:43Z

libcontainer/specconv/spec_linux.go

+		if spec.Linux.TimeOffsets != nil {
+			config.TimeOffsets = spec.Linux.TimeOffsets
+		}


Here if is not needed -- worst case scenario is you assign nil to config.TimeOffsets.

(In other places if is needed to avoid nil dereferences, but not here).

chethanah · 2023-07-31T03:52:10Z

Thank you for detailed review. I'll update as per comments.

cyphar · 2023-08-01T06:38:06Z

libcontainer/container_linux.go

+	// write boottime and monotonic time ns offsets.
+	if c.config.TimeOffsets != nil {
+		timeOffset := append([]byte("monotonic"), []byte(fmt.Sprintf(" %d %d\n", c.config.TimeOffsets["monotonic"].Secs, c.config.TimeOffsets["monotonic"].Nanosecs))...)
+		timeOffset = append(timeOffset, []byte("boottime")...)
+		r.AddData(&Bytemsg{
+			Type:  TimeOffsetsAttr,
+			Value: append(timeOffset, []byte(fmt.Sprintf(" %d %d", c.config.TimeOffsets["boottime"].Secs, c.config.TimeOffsets["boottime"].Nanosecs))...),
+		})
+	}


This is a little difficult to read. Maybe something more like this (untested):

Suggested change

// write boottime and monotonic time ns offsets.

if c.config.TimeOffsets != nil {

timeOffset := append([]byte("monotonic"), []byte(fmt.Sprintf(" %d %d\n", c.config.TimeOffsets["monotonic"].Secs, c.config.TimeOffsets["monotonic"].Nanosecs))...)

timeOffset = append(timeOffset, []byte("boottime")...)

r.AddData(&Bytemsg{

Type: TimeOffsetsAttr,

Value: append(timeOffset, []byte(fmt.Sprintf(" %d %d", c.config.TimeOffsets["boottime"].Secs, c.config.TimeOffsets["boottime"].Nanosecs))...),

})

}

// write boottime and monotonic time ns offsets.

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)

}

r.AddData(&Bytemsg{

Type: TimeOffsetsAttr,

Value: offsetSpec.Bytes(),

})

}

In Go, printf-like functions are notoriously slow for some reason. I'd prefer string concatenation, but it's not a big deal since it's only done once during the container start here.

libcontainer/nsenter/nsexec.c

libcontainer/container_linux.go

"time" namespace was introduced in Linux v5.6 support new time namespace to set boottime and monotonic time offset Example runtime spec "timeOffsets": { "monotonic": { "secs": 172800, "nanosecs": 0 }, "boottime": { "secs": 604800, "nanosecs": 0 } } Signed-off-by: Chethan Suresh <chethan.suresh@sony.com>

cyphar

lgtm

lifubang · 2023-08-06T07:24:35Z

libcontainer/nsenter/nsexec.c

+		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)


EPERM means The caller does not have the CAP_SYS_TIME capability.
Why ignore this error?

Good catch. I think this was mistakenly copied from the /proc/self/uid_map and gid_map helpers (which call the setuid newuidmap/newgidmap helper programs in case of -EPERM).

cyphar · 2023-08-06T09:16:28Z

libcontainer/nsenter/nsexec.c

+		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)


Good catch. I think this was mistakenly copied from the /proc/self/uid_map and gid_map helpers (which call the setuid newuidmap/newgidmap helper programs in case of -EPERM).

cyphar · 2023-08-06T09:18:29Z

libcontainer/nsenter/nsexec.c

+	if (write_file(map, map_len, "/proc/self/timens_offsets") < 0) {
+		if (errno != EPERM)
+			bail("failed to update /proc/self/timens_offsets");
+	}


Suggested change

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");

cyphar · 2023-08-06T09:21:07Z

libcontainer/container_linux.go

@@ -2321,6 +2321,18 @@ 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 {


I suspect we want this check to be done in the validator (at is stands, the code will silently ignore timensOffsets being configured without CLONE_NEWTIME enabled). But I can do this in a separate PR when I add the necessary integration tests for this feature.

libcontainer/container_linux.go

kolyshkin · 2023-08-10T00:40:42Z

libcontainer/specconv/spec_linux.go

+
+		// update timens offsets
+		config.TimeOffsets = spec.Linux.TimeOffsets


Nit: perhaps you can move this to be right after these lines:

config.MaskPaths = spec.Linux.MaskedPaths config.ReadonlyPaths = spec.Linux.ReadonlyPaths config.MountLabel = spec.Linux.MountLabel config.Sysctl = spec.Linux.Sysctl

and drop the comment since it's kind of obvious what we do here.

kolyshkin

Looks mostly OK to me, left a couple of nits.

cyphar

We can just merge this, I'll send a PR to fix up the remaining nits (including the incorrect EPERM ignore logic) so we don't need to do more rounds of review.

Thanks @chethanah!

chethanah changed the title ~~[WIP] Support time namespace~~ Support time namespace Jun 2, 2023

chethanah force-pushed the support-time-ns branch from 493a08f to c6f73ac Compare July 6, 2023 04:27

lifubang reviewed Jul 7, 2023

View reviewed changes

libcontainer/specconv/spec_linux.go Outdated Show resolved Hide resolved

chethanah commented Jul 11, 2023

View reviewed changes

libcontainer/container_linux.go Outdated Show resolved Hide resolved

chethanah force-pushed the support-time-ns branch 2 times, most recently from c87c035 to d6c0d52 Compare July 11, 2023 10:42

chethanah force-pushed the support-time-ns branch from 79e1968 to b16765e Compare July 16, 2023 18:08

AkihiroSuda added this to the 1.2.0 milestone Jul 16, 2023

AkihiroSuda reviewed Jul 17, 2023

View reviewed changes

libcontainer/container_linux.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Jul 17, 2023

View reviewed changes

libcontainer/specconv/spec_linux.go Outdated Show resolved Hide resolved

chethanah force-pushed the support-time-ns branch from b16765e to 9a5d7b4 Compare July 18, 2023 09:20

cyphar requested changes Jul 20, 2023

View reviewed changes

cyphar reviewed Jul 20, 2023

View reviewed changes

kolyshkin reviewed Jul 28, 2023

View reviewed changes

chethanah force-pushed the support-time-ns branch from 9a5d7b4 to f644e9a Compare August 1, 2023 05:43

cyphar requested changes Aug 1, 2023

View reviewed changes

chethanah commented Aug 1, 2023

View reviewed changes

libcontainer/nsenter/nsexec.c Show resolved Hide resolved

chethanah force-pushed the support-time-ns branch from f644e9a to 3791a45 Compare August 1, 2023 10:01

cyphar reviewed Aug 1, 2023

View reviewed changes

libcontainer/container_linux.go Outdated Show resolved Hide resolved

chethanah force-pushed the support-time-ns branch from 3791a45 to d29ead9 Compare August 3, 2023 02:43

chethanah force-pushed the support-time-ns branch from d29ead9 to ebc2e7c Compare August 3, 2023 04:43

cyphar approved these changes Aug 4, 2023

View reviewed changes

cyphar mentioned this pull request Aug 4, 2023

runc: release 1.2.0-rc.1 #3963

Closed

cyphar requested review from kolyshkin, AkihiroSuda and lifubang August 6, 2023 01:43

lifubang added the status/2-code-review label Aug 6, 2023

lifubang reviewed Aug 6, 2023

View reviewed changes

cyphar requested changes Aug 6, 2023

View reviewed changes

kolyshkin reviewed Aug 10, 2023

View reviewed changes

libcontainer/container_linux.go Show resolved Hide resolved

kolyshkin reviewed Aug 10, 2023

View reviewed changes

kolyshkin approved these changes Aug 10, 2023

View reviewed changes

cyphar approved these changes Aug 10, 2023

View reviewed changes

cyphar closed this in 0866112 Aug 10, 2023

cyphar merged commit 0866112 into opencontainers:main Aug 10, 2023
36 checks passed

cyphar mentioned this pull request Aug 10, 2023

timens: minor cleanups #3980

Merged

thaJeztah linked an issue Sep 13, 2023 that may be closed by this pull request

RFC: remove user/group name support from libcontainer #3998

Open

thaJeztah removed a link to an issue Sep 13, 2023

RFC: remove user/group name support from libcontainer #3998

Open

thaJeztah mentioned this pull request Sep 13, 2023

Support time namespaces #2345

Closed

lifubang mentioned this pull request Dec 13, 2023

(u|g)idMappings should not exist when joining an existing user ns #4122

Closed

cyphar mentioned this pull request Mar 14, 2024

release 1.2.0-rc.1 #4221

Merged

kwilczynski mentioned this pull request Apr 17, 2024

Time Sync problems after restore VM/Node from snapshot cri-o/cri-o#8033

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support time namespace #3876

Support time namespace #3876

chethanah commented May 22, 2023 •

edited by thaJeztah

Loading

chethanah commented May 22, 2023

chethanah commented May 22, 2023

chethanah commented May 23, 2023

AkihiroSuda commented Jun 10, 2023

chethanah commented Jul 11, 2023

chethanah commented Jul 12, 2023

cyphar left a comment •

edited

Loading

cyphar Jul 20, 2023

cyphar Jul 20, 2023

cyphar Jul 20, 2023

kolyshkin Jul 28, 2023

kolyshkin Jul 28, 2023

kolyshkin Jul 28, 2023

kolyshkin Jul 28, 2023

kolyshkin Jul 28, 2023

chethanah commented Jul 31, 2023

cyphar Aug 1, 2023

kolyshkin Aug 10, 2023

cyphar left a comment

lifubang Aug 6, 2023

cyphar Aug 6, 2023

cyphar Aug 6, 2023

cyphar Aug 6, 2023

cyphar Aug 6, 2023

kolyshkin Aug 10, 2023

kolyshkin left a comment

cyphar left a comment

	TimeOffsets map[string]specs.LinuxTimeOffset `json:"timeOffsets,omitempty"`
	TimeOffsets map[string]specs.LinuxTimeOffset `json:"time_offsets,omitempty"`

		if c.config.TimeOffsets["boottime"].Nanosecs != 0 \|\| c.config.TimeOffsets["boottime"].Secs != 0 {
		b, err := encodeTimeNs(c.config.TimeOffsets["boottime"])


		// update timens offsets
		config.TimeOffsets = spec.Linux.TimeOffsets

Support time namespace #3876

Support time namespace #3876

Conversation

chethanah commented May 22, 2023 • edited by thaJeztah Loading

Background

Support time ns in runc

Unit test

Discussion points

chethanah commented May 22, 2023

chethanah commented May 22, 2023

chethanah commented May 23, 2023

AkihiroSuda commented Jun 10, 2023

chethanah commented Jul 11, 2023

chethanah commented Jul 12, 2023

cyphar left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chethanah commented Jul 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyphar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolyshkin left a comment

Choose a reason for hiding this comment

cyphar left a comment

Choose a reason for hiding this comment

chethanah commented May 22, 2023 •

edited by thaJeztah

Loading

cyphar left a comment •

edited

Loading