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

Support recursive mount attrs ("rro", "rnosuid", "rnodev", ...) #3272

Merged
merged 3 commits into from
Dec 8, 2021

Conversation

AkihiroSuda
Copy link
Member

@AkihiroSuda AkihiroSuda commented Nov 11, 2021

The new mount option "rro" makes the mount point recursively read-only, by calling mount_setattr(2) with MOUNT_ATTR_RDONLY and AT_RECURSIVE.
https://man7.org/linux/man-pages/man2/mount_setattr.2.html

Requires kernel >= 5.12.

The "rro" option string conforms to the proposal in util-linux/util-linux#1501.

Fix #2823


Similary, this commit also adds the following mount options:

  • rrw
  • r[no]{suid,dev,exec,relatime,atime,strictatime,diratime,symfollow}
  • [no]symfollow

libcontainer/utils/syscallutil/syscallutil.go Outdated Show resolved Hide resolved
libcontainer/configs/mount.go Outdated Show resolved Hide resolved
libcontainer/configs/mount.go Outdated Show resolved Hide resolved
libcontainer/rootfs_linux.go Outdated Show resolved Hide resolved
libcontainer/specconv/spec_linux.go Outdated Show resolved Hide resolved
libcontainer/utils/syscallutil/syscallutil.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need rebase on top of #3281

kolyshkin
kolyshkin previously approved these changes Nov 18, 2021
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

kolyshkin
kolyshkin previously approved these changes Nov 19, 2021
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kolyshkin
Copy link
Contributor

@cyphar PTAL

@AkihiroSuda AkihiroSuda marked this pull request as draft December 7, 2021 05:19
@AkihiroSuda
Copy link
Member Author

Marked as a draft to avoid "merge conflict" with #3296

@cyphar
Copy link
Member

cyphar commented Dec 7, 2021

#3296 is merged, will re-review this once it's updated to add the new options to runc features.

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
See MS_NOSYMFOLLOW in mount(2)

https://man7.org/linux/man-pages/man2/mount.2.html

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
The new mount option "rro" makes the mount point recursively read-only,
by calling `mount_setattr(2)` with `MOUNT_ATTR_RDONLY` and `AT_RECURSIVE`.
https://man7.org/linux/man-pages/man2/mount_setattr.2.html

Requires kernel >= 5.12.

The "rro" option string conforms to the proposal in util-linux/util-linux Issue 1501.

Fix issue 2823

Similary, this commit also adds the following mount options:
- rrw
- r[no]{suid,dev,exec,relatime,atime,strictatime,diratime,symfollow}

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
@AkihiroSuda
Copy link
Member Author

Updated. Ready for Review.

Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. We'll have to see how the runtime-spec changes go with regards to how we'll give full access to mount_setattr.

I also wanted to ask -- one of the reasons mount_setattr was introduced was because our rootfs read-only setup only made / and not any lower mount points read-only. Did we want to change that behaviour too at some point (keeping in mind we'd need to do it carefully)?

@AkihiroSuda
Copy link
Member Author

I also wanted to ask -- one of the reasons mount_setattr was introduced was because our rootfs read-only setup only made / and not any lower mount points read-only. Did we want to change that behaviour too at some point (keeping in mind we'd need to do it carefully)?

Do you mean .root.readonly should make all .[]mounts read-only?
That seems dangerous and needs an additional field like .root.readonlyRecursive in the Runtime Spec.

@cyphar
Copy link
Member

cyphar commented Dec 8, 2021

Do you mean .root.readonly should make all .[]mounts read-only? That seems dangerous and needs an additional field like .root.readonlyRecursive in the Runtime Spec.

Yes and yeah we'd probably need to do more runtime-spec work (though it should be noted in my experience a lot of runc users expect root.readonly to mean "make the whole container read-only" -- namely docker run --read-only -- though whether we should match that expectation is probably a separate question).

@cyphar cyphar requested review from kolyshkin and a team December 8, 2021 06:18
@cyphar cyphar mentioned this pull request Dec 8, 2021
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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.

Support recursively-readonly mounts with mount_setattr (kernel 5.12)
3 participants