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

Adding --no-canonicalize prevents user mounts #7294

Closed
behlendorf opened this issue Mar 12, 2018 · 8 comments
Closed

Adding --no-canonicalize prevents user mounts #7294

behlendorf opened this issue Mar 12, 2018 · 8 comments
Milestone

Comments

@behlendorf
Copy link
Member

@behlendorf behlendorf commented Mar 12, 2018

System information

Type Version/Name
Distribution Name Ubuntu
Distribution Version 16.04
Linux Kernel Any
Architecture x86_64
ZFS Version zfs-0.7.0-360-g8e5d148
SPL Version spl-0.7.0-30-g3673d03

Describe the problem you're observing

Reposted from #6437 (comment).

PR #6437 appears to prevent user-mode mounting of volumes.

Is it possible that the --no-canonicalize option could be used only when necessary? Or is there another solution to this that doesn't prevent user mounting?

Describe how to reproduce the problem

root@athena:~# zfs allow jules mount testpool/jtest

jules@athena:~$ zfs mount testpool/jtest
mount: only root can use "--no-canonicalize" option
cannot mount 'testpool/jtest': Invalid argument

Include any warning/errors/backtraces from the system logs

None

@behlendorf behlendorf added this to the 0.8.0 milestone Mar 12, 2018
@loli10K

This comment has been minimized.

Copy link
Contributor

@loli10K loli10K commented Mar 12, 2018

I did not know zfs allow mount could work with any other user than root, from zfs(8):

zfs allow filesystem|volume

Displays permissions that have been delegated on the specified filesystem or volume. See the other forms of zfs allow for more information.

Delegations are supported under Linux with the exception of mount, unmount, mountpoint, canmount, rename, and share. These permissions cannot be delegated because the Linux mount(8) command restricts modifications of the global namespace to the root user.

EDIT: i'm sorry if #6437 broke some functionality, but from the man page i thought this was a non-issue.

@bunder2015

This comment has been minimized.

Copy link
Contributor

@bunder2015 bunder2015 commented Mar 17, 2018

I would like to second what @loli10K has mentioned, zfs allow has also been a repeated topic of conversation on IRC. It would be nice if it was fully functional on linux. We seem to be in a chicken-and-egg problem where ZFS is blaming linux and linux is blaming ZFS.

edit: FWIW, it actually is possible for users to mount (non-zfs) filesystems if the fstab entry has the "user" mount option set.

As well, the zfs command is installed in /sbin which isn't a part of the default path on some distros.

There was also previously another problem due to the permissions on /dev/zfs.

@behlendorf

This comment has been minimized.

Copy link
Member Author

@behlendorf behlendorf commented Mar 19, 2018

@bunder2015 @loli10K it's definietly worth taking a second look at delegations for mount, unmount, etc. The landscape here may have changed somewhat since the initial implementation and we may be able to workaround, or live with, some of the restrictions Linux imposes. If someone has the time to work on this that would be great.

For example, if I recall correctly while Linux does allow "user" mounts it only done so when:

  1. The /etc/fstab contains the keyword "user", that means only legacy ZFS mount support, or
  2. The mount is allowed in a private user namespace, never in the global system namespace.

Adding support for the second case might address most peoples concerns since it should allow delegations to work correctly in containers.

As well, the zfs command is installed in /sbin which isn't a part of the default path on some distros.

Perhaps we can symlink it from /bin. It's installed by default in /sbin for FHS conformance reasons.

There was also previously another problem due to the permissions on /dev/zfs.

This should have been addressed by the updated udev rules some time ago.

@behlendorf behlendorf mentioned this issue Mar 19, 2018
4 of 7 tasks complete
@bunder2015

This comment has been minimized.

Copy link
Contributor

@bunder2015 bunder2015 commented Mar 20, 2018

Unfortunately legacy mount points won't work here since users can't edit /etc/fstab (without sudo/etc, at least).

However I just had a thought, the mount binary is setuid root, could we potentially leverage that for our use outside of containers?

@pstch

This comment has been minimized.

Copy link

@pstch pstch commented Nov 15, 2018

EDIT: I think this is a non-issue, you way want not to bother with these comments and jump to the last one.

I've been trying to find a solution to this problem, although I'm not an experienced filesystem developer.

I don't think that the mount behaviour will change (requiring root to disable canonicalization) any time soon, so it seems that the only solution to this problem is not using --no-canonicalize, or allowing not to use it.

This means that another solution has to be found for #1791 (presumably) and #6429. I understand that both of these issues are caused by the canonicalization of the device path.

  • Would it be possible (and wise) to handle the canonicalization of the path in ZFS itself (EDIT: in such a way that it's doesn't trigger #1791 and #6429), so that the canonicalization made by mount has no effect ? If yes, I think I can try to write the code for this.
  • If the above change is not possible (or wise), would a PR adding a configuration option that disables --no-canonicalize be acceptable ? This would probably require reverting the first part of #6437 (or use an empty directory as suggested somewhere else), and would expose the user (when the option is enabled) to the #1791 and #6429 bugs.
@pstch

This comment has been minimized.

Copy link

@pstch pstch commented Nov 15, 2018

After reading util-linux/lib/canonicalize.c, I conclude that the main cause of #1791 and #6429 is realpath, which assumes that the path is relative to the CWD. This triggers #1791 (because realpath will resolve symlinks) and #6429 (WorkingDirectory being required).

This actually looks hard to workaround in zfs mount. mount seems to always interpret its arguments as files, and requires sanitized paths for non-root users. I'm thinking that maybe something can be done by making zfs mount pass mount a special form of the device path, and handling this special form in mount.zfs, so that the canonicalization done in mount has no effect.

EDIT: maybe this prefix could be a simple /, added by zfs mount, making realpath a no-op, and then removed by mount.zfs. I will run some experiments on this tomorrow. This could cause a problem if a ZFS pool is imported from a file in the root directory that has the same path as the dataset being mounted (as mount.zfs would mount the ZFS pool imported from the file in the root directory rather than the requested dataset). So /nonexistent/ maybe ? Or /zfs/ ? Or /dev/zfs/ (as /dev/zfs will never be a directory) ?

It seems like the core of the problem is that the interface exposed by the mount command requires paths for sources, while mount only requires a character string. Why does zfs mount need to defer to mount to call mount.zfs ?

@pstch

This comment has been minimized.

Copy link

@pstch pstch commented Nov 18, 2018

This actually seems to be a non-issue. --no-canonicalize doesn't prevent much as --options cannot be used by non-root users either. I don't see how adding it prevented anything, and trying to reproduce the problem without --no-canonicalize returns the same error but for --options.

Also, I don't think this is related to #6865, as mounts in an user namespace can be done by the root user in that namespace (mount checks the inner uid/gid). Allowing zfs mount in unprivileged containers/user namespaces does not require removing --no-canonicalize.

I think that --no-canonicalize should be accepted as the proper solution for the issues it fixed, (especially as the source is not a path when using ZFS, so canonicalizing it is a bad idea anyway.) and that this issue should be closed.

@behlendorf

This comment has been minimized.

Copy link
Member Author

@behlendorf behlendorf commented Dec 4, 2018

@pstch thanks for digging in to this one. Since I originally opened this one I'm happy to close it out as a non-issue.

@behlendorf behlendorf closed this Dec 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.