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

WIP: libcontainer/specconv/spec_linux: defaults should not be a no-op #1444

Closed
wants to merge 1 commit into from

Conversation

wking
Copy link
Contributor

@wking wking commented May 11, 2017

It has been since it landed in 9fac183, but the spec currently references mount(8) for these options and mount(8) has:

  • defaults

    Use the default options: rw, suid, dev, exec, auto, nouser, and async.

    Note that the real set of all default mount options depends on kernel and filesystem type. See the beginning of this section for more details.

I exepect that “real set” paragraph applies to:

Note that filesystems also have per-filesystem specific default mount options (see for example tune2fs -l output for extN filesystems).

For what its worth, util-linux 2.28.2 seems to ignore defaults instead of clearing bits:

# strace -o /tmp/trace mount -t tmpfs -o ro,defaults - /tmp/a
# grep 'mount(' /tmp/trace
mount("-", "/tmp/a", "tmpfs", MS_MGC_VAL|MS_RDONLY, NULL) = 0

While a single-bit clear option does unset an earlier bit:

# strace -o /tmp/trace mount -t tmpfs -o ro,rw - /tmp/a
# grep 'mount(' /tmp/trace
mount("-", "/tmp/a", "tmpfs", MS_MGC_VAL, NULL) = 0

but the spec is currnently punting to the util-linux mount(8) page and not to the util-linux implementation.

Related to opencontainers/runtime-spec#771.

@wking wking force-pushed the defaults-mount-option branch 2 times, most recently from 435b0c7 to a6c4f38 Compare May 11, 2017 08:11
It has been since it landed in 9fac183 (Initial commit of runc
binary, 2015-06-21), but the spec currently references mount(8) for
these options [1] and mount(8) has:

  defaults
    Use the default options: rw, suid, dev, exec, auto, nouser, and
    async.

    Note that the real set of all default mount options depends on
    kernel and filesystem type.  See the beginning of this section for
    more details.

I exepect that "real set" paragraph applies to:

  Note that filesystems also have per-filesystem specific default
  mount options (see for example tune2fs -l output for extN
  filesystems).

This commit sets up 'defaults' according to that option list, but does
not do anything about 'auto' or 'nouser', which do not map to MS_*
flags and only apply to fstab entries.

For what its worth, util-linux 2.28.2 seems to ignore 'defaults'
instead of clearing bits:

  # strace -o /tmp/trace mount -t tmpfs -o ro,defaults - /tmp/a
  # grep 'mount(' /tmp/trace
  mount("-", "/tmp/a", "tmpfs", MS_MGC_VAL|MS_RDONLY, NULL) = 0

While a single-bit clear option does unset an earlier bit:

  # strace -o /tmp/trace mount -t tmpfs -o ro,rw - /tmp/a
  # grep 'mount(' /tmp/trace
  mount("-", "/tmp/a", "tmpfs", MS_MGC_VAL, NULL) = 0

but the spec is currnently punting to the util-linux mount(8) page and
not to the util-linux implementation.

[1]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0-rc5/config.md#L68
[2]: http://man7.org/linux/man-pages/man8/mount.8.html#FILESYSTEM-INDEPENDENT_MOUNT%20OPTIONS

Signed-off-by: W. Trevor King <[email protected]>
@wking wking force-pushed the defaults-mount-option branch from a6c4f38 to 4999aba Compare May 11, 2017 10:17
@wking wking changed the title libcontainer/specconv/spec_linux: defaults should not be a no-op WIP: libcontainer/specconv/spec_linux: defaults should not be a no-op May 11, 2017
@wking
Copy link
Contributor Author

wking commented May 11, 2017

I've posted a request for clarification to the util-linux list to resolve the differences between my reading of the man page and the util-linux implementation.

@wking
Copy link
Contributor Author

wking commented May 11, 2017

Updated mount(8) wording is still up in the air, but Karel Zak confirmed that runC's current defaults is correct and my interpretation here is wrong.

@wking wking closed this May 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant