mount: use the mount syscall directly #10294
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10294 +/- ##
==========================================
- Coverage 79.46% 79.44% -0.02%
==========================================
Files 389 389
Lines 123120 123137 +17
==========================================
- Hits 97834 97831 -3
- Misses 25286 25306 +20
Continue to review full report at Codecov.
|
felixdoerre
force-pushed
the
zfs_raw_mount
branch
2 times, most recently
from
91eaf6c to
a92131c
Compare
behlendorf
added
the
Status: Design Review Needed
felixdoerre
force-pushed
the
zfs_raw_mount
branch
from
a92131c to
09538bf
Compare
felixdoerre
changed the title
felixdoerre
force-pushed
the
zfs_raw_mount
branch
from
09538bf to
e6bc6db
Compare
|
@grwilson @tuxoko would you mind taking a look at this PR. It tackles some work you've both looked at in the past regarding having libzfs directly call mount(2). The short version is that a lot of the issues which originally prevented us from doing this are no longer relevant for current Linux systems. The major one being the need to update an There is still a legitimate concern around possible problems when not using a mount helper on systems with selinux enabled. But in that case I think it's reasonable to fall back to the mount helper if needed. @prometheanfire as a selinux user would it be possible for you to try out the PR and see if there are any issues? Testing was done on CentOS 8 with it enforcing and everything seemed to work, but it would be nice to know that's the case for other environments. Thanks in advance! |
| */ | ||
| int | ||
| zfs_parse_mount_options(char *mntopts, unsigned long *mntflags, | ||
| unsigned long *zfsflags, int sloppy, char *badopt, char *mtabopt) |
There was a problem hiding this comment.
Now that there are multiple callers it would be nice to allow NULL to be passed for badopt and mtabopt, in which case those values simply aren't set.
There was a problem hiding this comment.
I have let badopt be as it is, as the option that caused parsing to fail should probably printed as an error. Can I just copy something like this:
return (zfs_error_fmt(hdl, ENXIO
dgettext(TEXT_DOMAIN, "cannot mount: option '%s' unknown"),
badopt)
Would I need to extend do_mount with an additional libzfs_handle_t parameter, or can I somehow use the already give zfs_handle_t parameter?
There was a problem hiding this comment.
The way this is structured we'd want to add the special error message to the error handling block at the end of zfs_mount_at(). While it'd be nice to provide a more useful error message with the bad argument I don't think it's critical. But what we do need to do is update do_mount() so it returns EINVAL and not ENXIO in this case. This way we'll at least print a meaningful generic error, instead of a misleading errno and message.
|
my systemd env is kinda defunct, but if it worked on cent-8 it should work anywhere else, the policies are more or less central across distros. |
felixdoerre
force-pushed
the
zfs_raw_mount
branch
2 times, most recently
from
cb1cbb4 to
1115a1f
Compare
Codecov Report
@@ Coverage Diff @@
## master #10294 +/- ##
==========================================
+ Coverage 79.57% 79.90% +0.32%
==========================================
Files 390 390
Lines 123350 123364 +14
==========================================
+ Hits 98153 98571 +418
+ Misses 25197 24793 -404
Continue to review full report at Codecov.
|
| */ | ||
| int | ||
| zfs_parse_mount_options(char *mntopts, unsigned long *mntflags, | ||
| unsigned long *zfsflags, int sloppy, char *badopt, char *mtabopt) |
There was a problem hiding this comment.
The way this is structured we'd want to add the special error message to the error handling block at the end of zfs_mount_at(). While it'd be nice to provide a more useful error message with the bad argument I don't think it's critical. But what we do need to do is update do_mount() so it returns EINVAL and not ENXIO in this case. This way we'll at least print a meaningful generic error, instead of a misleading errno and message.
Signed-off-by: Felix Dörre <felix@dogcraft.de>
felixdoerre
force-pushed
the
zfs_raw_mount
branch
from
acd7701 to
f0aed99
Compare
behlendorf
added
Status: Accepted
Allow zfs datasets to be mounted on Linux without relying on the invocation of an external processes. This is the same behavior which is implemented for FreeBSD. Use of the libmount library was originally considered because it provides functionality to properly lock and update the /etc/mtab file. However, these days /etc/mtab is typically a symlink to /proc/self/mounts so there's nothing to updated. Therefore, we call mount(2) directly and avoid any additional dependencies. If required the legacy behavior can be enabled by setting the ZFS_MOUNT_HELPER environment variable. This may be needed in environments where SELinux in enabled and the zfs binary does not have mount permission. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Felix Dörre <felix@dogcraft.de> openzfs#10294
This is just a draft change to understand if/how using mount directly breaks any test cases.
Motivation and Context
The motivation behind this PR is to be able to mount zfs datasets without the invocation of external processes. This allows the mounting to be used from inside a pam module. Spawning processes from within a pam module is highly discouraged as the application might be confused by the additional child processes.
Description
How Has This Been Tested?
For now some mount-related unit tests have been executed locally and they passed.
Types of changes
I am not sure, how I would classify this PR. It can be seen both as a new feature but also as a cleanup/bugfix kind of change.
Checklist:
Signed-off-by.