Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config-linux: Clearer punt to kernel for linux.devices #829

Closed

Conversation

wking
Copy link
Contributor

@wking wking commented May 15, 2017

This is a bit awkward, since:

  • It's not a direct wrapper around mknod(2) (which, for example, does not use the c/b/u/p characters).
  • The runtime doesn't have to use mknod(2), so binding it to mknod(1)-ish invocations doesn't make much sense.

Instead, I've bound it to POSIX's stat(3) to show what compliance testing (and anything else inside the container) can expect the results (however the runtime accomplishes them) to look like.

The previous wording wasn't clear on whether symlinks were an allowed approach. The new wording explicitly allows them by using stat(1)-like symlink resolution.

I've also clarified relative path handling and explicitly declared the appropriate mount namespace (impacts path) and PID namespace (impacts uid and gid).

Because we're focused on post-create stat calls, I've also added new wording about handling duplicate path values.

I've used POSIX reference where possible (vs. Linux man pages), because they contain sufficient detail for this section, have well-versioned URLs, and are more likely to be portable if this section ever applies to non-Linux configs (BSD? Solaris?).

Related to recent discussion around #690, #780 and punting to the kernel, although in this case we're not changing the JSON Schema because the existing local validation (valid type characters and the fileMode range) both feed into a single mode_t integer in the stat(3) and mknod(2) APIs. For a cleaner kernel punt, we could drop type, lift the range limit on fileMode, and map it directly to st.st_mode. But that seemed like a big backwards-compat shift for this PR.

@vbatts
Copy link
Member

vbatts commented May 24, 2017

Adding this to the label for a next release. If you feel particular for 1.0.0, state so.

@wking
Copy link
Contributor Author

wking commented May 24, 2017 via email

@wking wking force-pushed the linux-device-strict-kernel-punt branch from 89b13e5 to b486f4b Compare June 1, 2017 15:03
This is a bit awkward, since:

* It's not a direct wrapper around mknod(2) (which, for example, does
  not use the c/b/u/p characters).
* The runtime doesn't have to use mknod, so binding it to mknod(1)-ish
  invocations doesn't make much sense.

Instead, I've bound it to POSIX's stat(3) to show what compliance
testing (and anything else inside the container) can expect the
results (however the runtime accomplishes them) to look like.

The previous wording wasn't clear on whether symlinks were an allowed
approach.  The new wording explicitly allows them by using
stat(1)-like symlink resolution.

I've also clarified relative 'path' handling and explicitly declared
the appropriate mount namespace (impacts 'path') and PID namespace
(impacts 'uid' and 'gid').

Because we're focused on post-create stat calls, I've also added new
wording about handling duplicate 'path' values.

I've used POSIX reference where possible (vs. Linux man pages),
because they contain sufficient detail for this section, have
well-versioned URLs, and are more likely to be portable if this
section ever applies to non-Linux configs (BSD?  Solaris?).

Related to recent discussion around punting to the kernel [1,2],
although in this case we're not changing the JSON Schema because the
existing local validation (valid 'type' characters and the 'fileMode'
range) both feed into a single mode_t integer in the stat(3) and
mknod(2) APIs.  For a cleaner kernel punt, we could drop 'type', lift
the range limit on 'fileMode', and map it directly to st.st_mode. But
that seemed like a big backwards-compat shift for this commit.

[1]: opencontainers#780
[2]: opencontainers#690 (comment)

Signed-off-by: W. Trevor King <[email protected]>
@wking
Copy link
Contributor Author

wking commented Jun 1, 2017

Rebased around #846 with 89b13e5b486f4b.

@crosbymichael
Copy link
Member

We don't want to bind the spec to kernel headers and specific implementations.

@wking
Copy link
Contributor Author

wking commented Jun 6, 2017

We don't want to bind the spec to kernel headers and specific implementations.

This was not binding the spec to kernel headers, it was binding it to headers defined by POSIX. See also “I've used POSIX reference where possible…” in the topic post for this PR.

The only non-POSIX portion of this PR is major(3) and minor(3), because major/minor numbers are not a POSIX concept. For example, pax has to use the very weak:

In this case the devmajor and devminor fields shall contain information defining the device, the format of which is unspecified by this volume of POSIX.1-2008. Implementations may map the device specifications to their own local specification or may ignore the entry.

So the only way to access those major/minor numbers is through some Linux-specific interface like major(3)/minor(3) or bit-shifting (like runtime-tools does).

wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Jun 6, 2017
I'd rather address runtime compliance by breaking this down into
explicit checks based on POSIX stat(3) calls.  But with that approach
rejected [1], mentioning symlinks here helps motivate runtime-tools'
choice of os.Stat [2,3] (which follows symlinks) vs. os.Lstat (which
does not [4]).

[1]: opencontainers#829 (comment)
[2]: https://github.com/opencontainers/runtime-tools/blob/f5c82b3918bdfc3ed4b594dcfab4d1554beaf992/cmd/runtimetest/main.go#L319
[3]: https://golang.org/pkg/os/#Stat
[4]: https://golang.org/pkg/os/#Lstat

Signed-off-by: W. Trevor King <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants