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

[ws-daemon] Add support for idmapped mounts #14026

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

csweichel
Copy link
Contributor

Description

Adds support for idmapped mounts, alongside shiftfs and fuse-overlayfs

Related Issue(s)

Fixes #10181

How to test

  1. Set up a workspace preview machine with Linux 5.19
  2. Configure ws-daemon to use idmapped as fs shift method
  3. Start a workspace

Release Notes

[ws-daemon] Add support for ID mapped mounts

Documentation

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide

@csweichel
Copy link
Contributor Author

Turns out one cannot use idmapped mounts on top of overlayfs. It works as lower, but the mountSetAttr call on an overlayfs file descriptor fails. E.g. using mount-idmapped

$ mount -t overlay overlay -o lowerdir=a/lo,upperdir=a/up,workdir=a/work a/mnt
$ strace ./mount-idmapped --map-mount b:0:10000:10000 $PWD/a/mnt $PWD/mapped

mount_setattr(3, "", AT_EMPTY_PATH|AT_RECURSIVE, {attr_set=MOUNT_ATTR_IDMAP, attr_clr=0, propagation=0 /* MS_??? */, userns_fd=4}, 32) = -1 EINVAL (Invalid argument)

@stale
Copy link

stale bot commented Oct 30, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the meta: stale This issue/PR is stale and will be closed soon label Oct 30, 2022
@aledbf aledbf added meta: never-stale This issue can never become stale and removed meta: stale This issue/PR is stale and will be closed soon labels Oct 30, 2022
return xerrors.Errorf("open(userns): %w", err)
}
defer usernsFD.Close()
err = unix.MountSetattr(mappedFD, "", unix.AT_EMPTY_PATH|unix.AT_RECURSIVE, &unix.MountAttr{
Copy link
Contributor

Choose a reason for hiding this comment

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

@csweichel
How about this one? Maybe it could work without EINVAL because the id-mapped mount doesn't need to mount the overlays layers. Or did it have to be recursive?

Suggested change
err = unix.MountSetattr(mappedFD, "", unix.AT_EMPTY_PATH|unix.AT_RECURSIVE, &unix.MountAttr{
err = unix.MountSetattr(mappedFD, "", unix.AT_EMPTY_PATH, &unix.MountAttr{

Copy link
Contributor

Choose a reason for hiding this comment

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

@utam0k I assume you are making this suggestion to make it possible for us to use ID mapped mounts? Assuming yes, perhaps later this week, after deploy is done, you can spend <4 hours testing this in a workspace-preview?

Copy link
Contributor

Choose a reason for hiding this comment

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

Roger that.

@utam0k
Copy link
Contributor

utam0k commented Dec 20, 2022

Turns out one cannot use idmapped mounts on top of overlayfs. It works as lower, but the mountSetAttr call on an overlayfs file descriptor fails. E.g. using mount-idmapped

$ mount -t overlay overlay -o lowerdir=a/lo,upperdir=a/up,workdir=a/work a/mnt
$ strace ./mount-idmapped --map-mount b:0:10000:10000 $PWD/a/mnt $PWD/mapped

mount_setattr(3, "", AT_EMPTY_PATH|AT_RECURSIVE, {attr_set=MOUNT_ATTR_IDMAP, attr_clr=0, propagation=0 /* MS_??? */, userns_fd=4}, 32) = -1 EINVAL (Invalid argument)

For some reason, this sample code always uses AT_RECURSIVE even if the recursive flag is false.
https://github.com/brauner/mount-idmapped/blob/71a9e8bae308aed5aa59e02875122a728cdb5dba/mount-idmapped.c#L754

I believe we have to do like that. This code comes from the man of mount_setattr(2),

           ret = mount_setattr(fd_tree, "",
                               AT_EMPTY_PATH | (recursive ? AT_RECURSIVE : 0),
                               attr, sizeof(struct mount_attr));

@aledbf
Copy link
Member

aledbf commented Jan 5, 2023

For some reason, this sample code always uses AT_RECURSIVE even if the recursive flag is false.

@utam0k that is included in the util-linux change

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 for idmapped mounts instead of shiftfs
5 participants