* [PATCH v4 0/3] virtiofsd: Add support to enable/disable posix acls @ 2021-02-24 16:58 ` Vivek Goyal 0 siblings, 0 replies; 8+ messages in thread From: Vivek Goyal @ 2021-02-24 16:58 UTC (permalink / raw) To: qemu-devel, virtio-fs; +Cc: lhenriques, stefanha, dgilbert, vgoyal, miklos Hi, This is V4 of patches. In V3 I enabled posix_acl by default. Then I tested with cache=none and a simple "cat foo.txt" results in two extra GETXATTR(system.posix_acl) calls. (Note, this happens only if caller is not owner of file). And this happens on every call of "cat foo.txt" because cache=none does not cache anything in guest. So this make me rethink the idea of enabling posix_acl by default. I prefer not to have any performance regressions in any of the mode out of the box. Hence proposing this V4 where posix_acls are disabled by default and user needs to enable it explicitly if the want posix acls. This probably can change down the line once somebody can prove enabling it by deafult really does not impact us much. Change since V3. - Do not enable posix_acl by default. - Error out if user wants to enable it but xattr are disabled. - Modified help message and docs/tools/virtiofsd.rst to add documentation for this new knob (Luis Henriques). - Captured Reviewed-by tags from Stefan from V3. Thanks Vivek Vivek Goyal (3): virtiofsd: Add umask to seccom allow list virtiofsd: Add capability to change/restore umask virtiofsd: Add an option to enable/disable posix acls docs/tools/virtiofsd.rst | 3 + tools/virtiofsd/helper.c | 1 + tools/virtiofsd/passthrough_ll.c | 125 ++++++++++++++++++++++++-- tools/virtiofsd/passthrough_seccomp.c | 1 + 4 files changed, 123 insertions(+), 7 deletions(-) -- 2.25.4 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Virtio-fs] [PATCH v4 0/3] virtiofsd: Add support to enable/disable posix acls @ 2021-02-24 16:58 ` Vivek Goyal 0 siblings, 0 replies; 8+ messages in thread From: Vivek Goyal @ 2021-02-24 16:58 UTC (permalink / raw) To: qemu-devel, virtio-fs; +Cc: vgoyal, miklos Hi, This is V4 of patches. In V3 I enabled posix_acl by default. Then I tested with cache=none and a simple "cat foo.txt" results in two extra GETXATTR(system.posix_acl) calls. (Note, this happens only if caller is not owner of file). And this happens on every call of "cat foo.txt" because cache=none does not cache anything in guest. So this make me rethink the idea of enabling posix_acl by default. I prefer not to have any performance regressions in any of the mode out of the box. Hence proposing this V4 where posix_acls are disabled by default and user needs to enable it explicitly if the want posix acls. This probably can change down the line once somebody can prove enabling it by deafult really does not impact us much. Change since V3. - Do not enable posix_acl by default. - Error out if user wants to enable it but xattr are disabled. - Modified help message and docs/tools/virtiofsd.rst to add documentation for this new knob (Luis Henriques). - Captured Reviewed-by tags from Stefan from V3. Thanks Vivek Vivek Goyal (3): virtiofsd: Add umask to seccom allow list virtiofsd: Add capability to change/restore umask virtiofsd: Add an option to enable/disable posix acls docs/tools/virtiofsd.rst | 3 + tools/virtiofsd/helper.c | 1 + tools/virtiofsd/passthrough_ll.c | 125 ++++++++++++++++++++++++-- tools/virtiofsd/passthrough_seccomp.c | 1 + 4 files changed, 123 insertions(+), 7 deletions(-) -- 2.25.4 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v4 1/3] virtiofsd: Add umask to seccom allow list 2021-02-24 16:58 ` [Virtio-fs] " Vivek Goyal @ 2021-02-24 16:58 ` Vivek Goyal -1 siblings, 0 replies; 8+ messages in thread From: Vivek Goyal @ 2021-02-24 16:58 UTC (permalink / raw) To: qemu-devel, virtio-fs; +Cc: lhenriques, stefanha, dgilbert, vgoyal, miklos Patches in this series are going to make use of "umask" syscall. So allow it. Signed-off-by: Vivek Goyal <vgoyal@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- tools/virtiofsd/passthrough_seccomp.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/virtiofsd/passthrough_seccomp.c b/tools/virtiofsd/passthrough_seccomp.c index 62441cfcdb..f49ed94b5e 100644 --- a/tools/virtiofsd/passthrough_seccomp.c +++ b/tools/virtiofsd/passthrough_seccomp.c @@ -114,6 +114,7 @@ static const int syscall_allowlist[] = { SCMP_SYS(utimensat), SCMP_SYS(write), SCMP_SYS(writev), + SCMP_SYS(umask), }; /* Syscalls used when --syslog is enabled */ -- 2.25.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Virtio-fs] [PATCH v4 1/3] virtiofsd: Add umask to seccom allow list @ 2021-02-24 16:58 ` Vivek Goyal 0 siblings, 0 replies; 8+ messages in thread From: Vivek Goyal @ 2021-02-24 16:58 UTC (permalink / raw) To: qemu-devel, virtio-fs; +Cc: vgoyal, miklos Patches in this series are going to make use of "umask" syscall. So allow it. Signed-off-by: Vivek Goyal <vgoyal@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- tools/virtiofsd/passthrough_seccomp.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/virtiofsd/passthrough_seccomp.c b/tools/virtiofsd/passthrough_seccomp.c index 62441cfcdb..f49ed94b5e 100644 --- a/tools/virtiofsd/passthrough_seccomp.c +++ b/tools/virtiofsd/passthrough_seccomp.c @@ -114,6 +114,7 @@ static const int syscall_allowlist[] = { SCMP_SYS(utimensat), SCMP_SYS(write), SCMP_SYS(writev), + SCMP_SYS(umask), }; /* Syscalls used when --syslog is enabled */ -- 2.25.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v4 2/3] virtiofsd: Add capability to change/restore umask 2021-02-24 16:58 ` [Virtio-fs] " Vivek Goyal @ 2021-02-24 16:58 ` Vivek Goyal -1 siblings, 0 replies; 8+ messages in thread From: Vivek Goyal @ 2021-02-24 16:58 UTC (permalink / raw) To: qemu-devel, virtio-fs; +Cc: lhenriques, stefanha, dgilbert, vgoyal, miklos When parent directory has default acl and a file is created in that directory, then umask is ignored and final file permissions are determined using default acl instead. (man 2 umask). Currently, fuse applies the umask and sends modified mode in create request accordingly. fuse server can set FUSE_DONT_MASK and tell fuse client to not apply umask and fuse server will take care of it as needed. With posix acls enabled, requirement will be that we want umask to determine final file mode if parent directory does not have default acl. So if posix acls are enabled, opt in for FUSE_DONT_MASK. virtiofsd will set umask of the thread doing file creation. And host kernel should use that umask if parent directory does not have default acls, otherwise umask does not take affect. Miklos mentioned that we already call unshare(CLONE_FS) for every thread. That means umask has now become property of per thread and it should be ok to manipulate it in file creation path. This patch only adds capability to change umask and restore it. It does not enable it yet. Next patch will add capability to enable it based on if user enabled posix_acl or not. This should fix fstest generic/099. Reported-by: Luis Henriques <lhenriques@suse.de> Signed-off-by: Vivek Goyal <vgoyal@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- tools/virtiofsd/passthrough_ll.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c index 58d24c0010..050142e568 100644 --- a/tools/virtiofsd/passthrough_ll.c +++ b/tools/virtiofsd/passthrough_ll.c @@ -120,6 +120,7 @@ struct lo_inode { struct lo_cred { uid_t euid; gid_t egid; + mode_t umask; }; enum { @@ -169,6 +170,8 @@ struct lo_data { /* An O_PATH file descriptor to /proc/self/fd/ */ int proc_self_fd; int user_killpriv_v2, killpriv_v2; + /* If set, virtiofsd is responsible for setting umask during creation */ + bool change_umask; }; static const struct fuse_opt lo_opts[] = { @@ -1075,7 +1078,8 @@ static void lo_lookup(fuse_req_t req, fuse_ino_t parent, const char *name) * ownership of caller. * TODO: What about selinux context? */ -static int lo_change_cred(fuse_req_t req, struct lo_cred *old) +static int lo_change_cred(fuse_req_t req, struct lo_cred *old, + bool change_umask) { int res; @@ -1095,11 +1099,14 @@ static int lo_change_cred(fuse_req_t req, struct lo_cred *old) return errno_save; } + if (change_umask) { + old->umask = umask(req->ctx.umask); + } return 0; } /* Regain Privileges */ -static void lo_restore_cred(struct lo_cred *old) +static void lo_restore_cred(struct lo_cred *old, bool restore_umask) { int res; @@ -1114,6 +1121,9 @@ static void lo_restore_cred(struct lo_cred *old) fuse_log(FUSE_LOG_ERR, "setegid(%u): %m\n", old->egid); exit(1); } + + if (restore_umask) + umask(old->umask); } static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent, @@ -1138,7 +1148,7 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent, return; } - saverr = lo_change_cred(req, &old); + saverr = lo_change_cred(req, &old, lo->change_umask && !S_ISLNK(mode)); if (saverr) { goto out; } @@ -1147,7 +1157,7 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent, saverr = errno; - lo_restore_cred(&old); + lo_restore_cred(&old, lo->change_umask && !S_ISLNK(mode)); if (res == -1) { goto out; @@ -1828,7 +1838,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name, return; } - err = lo_change_cred(req, &old); + err = lo_change_cred(req, &old, lo->change_umask); if (err) { goto out; } @@ -1839,7 +1849,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name, fd = openat(parent_inode->fd, name, fi->flags | O_CREAT | O_EXCL, mode); err = fd == -1 ? errno : 0; - lo_restore_cred(&old); + lo_restore_cred(&old, lo->change_umask); /* Ignore the error if file exists and O_EXCL was not given */ if (err && (err != EEXIST || (fi->flags & O_EXCL))) { -- 2.25.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Virtio-fs] [PATCH v4 2/3] virtiofsd: Add capability to change/restore umask @ 2021-02-24 16:58 ` Vivek Goyal 0 siblings, 0 replies; 8+ messages in thread From: Vivek Goyal @ 2021-02-24 16:58 UTC (permalink / raw) To: qemu-devel, virtio-fs; +Cc: vgoyal, miklos When parent directory has default acl and a file is created in that directory, then umask is ignored and final file permissions are determined using default acl instead. (man 2 umask). Currently, fuse applies the umask and sends modified mode in create request accordingly. fuse server can set FUSE_DONT_MASK and tell fuse client to not apply umask and fuse server will take care of it as needed. With posix acls enabled, requirement will be that we want umask to determine final file mode if parent directory does not have default acl. So if posix acls are enabled, opt in for FUSE_DONT_MASK. virtiofsd will set umask of the thread doing file creation. And host kernel should use that umask if parent directory does not have default acls, otherwise umask does not take affect. Miklos mentioned that we already call unshare(CLONE_FS) for every thread. That means umask has now become property of per thread and it should be ok to manipulate it in file creation path. This patch only adds capability to change umask and restore it. It does not enable it yet. Next patch will add capability to enable it based on if user enabled posix_acl or not. This should fix fstest generic/099. Reported-by: Luis Henriques <lhenriques@suse.de> Signed-off-by: Vivek Goyal <vgoyal@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- tools/virtiofsd/passthrough_ll.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c index 58d24c0010..050142e568 100644 --- a/tools/virtiofsd/passthrough_ll.c +++ b/tools/virtiofsd/passthrough_ll.c @@ -120,6 +120,7 @@ struct lo_inode { struct lo_cred { uid_t euid; gid_t egid; + mode_t umask; }; enum { @@ -169,6 +170,8 @@ struct lo_data { /* An O_PATH file descriptor to /proc/self/fd/ */ int proc_self_fd; int user_killpriv_v2, killpriv_v2; + /* If set, virtiofsd is responsible for setting umask during creation */ + bool change_umask; }; static const struct fuse_opt lo_opts[] = { @@ -1075,7 +1078,8 @@ static void lo_lookup(fuse_req_t req, fuse_ino_t parent, const char *name) * ownership of caller. * TODO: What about selinux context? */ -static int lo_change_cred(fuse_req_t req, struct lo_cred *old) +static int lo_change_cred(fuse_req_t req, struct lo_cred *old, + bool change_umask) { int res; @@ -1095,11 +1099,14 @@ static int lo_change_cred(fuse_req_t req, struct lo_cred *old) return errno_save; } + if (change_umask) { + old->umask = umask(req->ctx.umask); + } return 0; } /* Regain Privileges */ -static void lo_restore_cred(struct lo_cred *old) +static void lo_restore_cred(struct lo_cred *old, bool restore_umask) { int res; @@ -1114,6 +1121,9 @@ static void lo_restore_cred(struct lo_cred *old) fuse_log(FUSE_LOG_ERR, "setegid(%u): %m\n", old->egid); exit(1); } + + if (restore_umask) + umask(old->umask); } static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent, @@ -1138,7 +1148,7 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent, return; } - saverr = lo_change_cred(req, &old); + saverr = lo_change_cred(req, &old, lo->change_umask && !S_ISLNK(mode)); if (saverr) { goto out; } @@ -1147,7 +1157,7 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent, saverr = errno; - lo_restore_cred(&old); + lo_restore_cred(&old, lo->change_umask && !S_ISLNK(mode)); if (res == -1) { goto out; @@ -1828,7 +1838,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name, return; } - err = lo_change_cred(req, &old); + err = lo_change_cred(req, &old, lo->change_umask); if (err) { goto out; } @@ -1839,7 +1849,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name, fd = openat(parent_inode->fd, name, fi->flags | O_CREAT | O_EXCL, mode); err = fd == -1 ? errno : 0; - lo_restore_cred(&old); + lo_restore_cred(&old, lo->change_umask); /* Ignore the error if file exists and O_EXCL was not given */ if (err && (err != EEXIST || (fi->flags & O_EXCL))) { -- 2.25.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v4 3/3] virtiofsd: Add an option to enable/disable posix acls 2021-02-24 16:58 ` [Virtio-fs] " Vivek Goyal @ 2021-02-24 16:58 ` Vivek Goyal -1 siblings, 0 replies; 8+ messages in thread From: Vivek Goyal @ 2021-02-24 16:58 UTC (permalink / raw) To: qemu-devel, virtio-fs; +Cc: lhenriques, stefanha, dgilbert, vgoyal, miklos fuse has an option FUSE_POSIX_ACL which needs to be opted in by fuse server to enable posix acls. As of now we are not opting in for this, so posix acls are disabled on virtiofs by default. Add virtiofsd option "-o posix_acl/no_posix_acl" to let users enable/disable posix acl support. By default it is disabled as of now due to performance concerns with cache=none. Currently even if file server has not opted in for FUSE_POSIX_ACL, user can still query acl and set acl, and system.posix_acl_access and system.posix_acl_default xattrs show up listxattr response. Miklos said this is confusing. So he said lets block and filter system.posix_acl_access and system.posix_acl_default xattrs in getxattr/setxattr/listxattr if user has explicitly disabled posix acls using -o no_posix_acl. As of now continuing to keeping the existing behavior if user did not specify any option to disable acl support due to concerns about backward compatibility. Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- docs/tools/virtiofsd.rst | 3 + tools/virtiofsd/helper.c | 1 + tools/virtiofsd/passthrough_ll.c | 103 ++++++++++++++++++++++++++++++- 3 files changed, 106 insertions(+), 1 deletion(-) diff --git a/docs/tools/virtiofsd.rst b/docs/tools/virtiofsd.rst index 866b7db3ee..55f3bb8753 100644 --- a/docs/tools/virtiofsd.rst +++ b/docs/tools/virtiofsd.rst @@ -101,6 +101,9 @@ Options Enable/disable extended attributes (xattr) on files and directories. The default is ``no_xattr``. + * posix_acl|no_posix_acl - + Enable/disable posix acl support. Posix ACLs are disabled by default`. + .. option:: --socket-path=PATH Listen on vhost-user UNIX domain socket at PATH. diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c index 28243b51b2..800a1a6801 100644 --- a/tools/virtiofsd/helper.c +++ b/tools/virtiofsd/helper.c @@ -183,6 +183,7 @@ void fuse_cmdline_help(void) " to virtiofsd from guest applications.\n" " default: no_allow_direct_io\n" " -o announce_submounts Announce sub-mount points to the guest\n" + " -o posix_acl/no_posix_acl Enable/Disable posix_acl. (default: disabled)\n" ); } diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c index 050142e568..7931b8af32 100644 --- a/tools/virtiofsd/passthrough_ll.c +++ b/tools/virtiofsd/passthrough_ll.c @@ -172,6 +172,7 @@ struct lo_data { int user_killpriv_v2, killpriv_v2; /* If set, virtiofsd is responsible for setting umask during creation */ bool change_umask; + int user_posix_acl; }; static const struct fuse_opt lo_opts[] = { @@ -204,6 +205,8 @@ static const struct fuse_opt lo_opts[] = { { "announce_submounts", offsetof(struct lo_data, announce_submounts), 1 }, { "killpriv_v2", offsetof(struct lo_data, user_killpriv_v2), 1 }, { "no_killpriv_v2", offsetof(struct lo_data, user_killpriv_v2), 0 }, + { "posix_acl", offsetof(struct lo_data, user_posix_acl), 1 }, + { "no_posix_acl", offsetof(struct lo_data, user_posix_acl), 0 }, FUSE_OPT_END }; static bool use_syslog = false; @@ -664,6 +667,21 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn) conn->want &= ~FUSE_CAP_HANDLE_KILLPRIV_V2; lo->killpriv_v2 = 0; } + + if (lo->user_posix_acl == 1) { + /* + * User explicitly asked for this option. Enable it unconditionally. + * If connection does not have this capability, it should fail + * in fuse_lowlevel.c + */ + fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling posix acl\n"); + conn->want |= FUSE_CAP_POSIX_ACL | FUSE_CAP_DONT_MASK; + lo->change_umask = true; + } else { + /* User either did not specify anything or wants it disabled */ + fuse_log(FUSE_LOG_DEBUG, "lo_init: disabling posix_acl\n"); + conn->want &= ~FUSE_CAP_POSIX_ACL; + } } static void lo_getattr(fuse_req_t req, fuse_ino_t ino, @@ -2622,6 +2640,63 @@ static int xattr_map_server(const struct lo_data *lo, const char *server_name, return -ENODATA; } +static bool block_xattr(struct lo_data *lo, const char *name) +{ + /* + * If user explicitly enabled posix_acl or did not provide any option, + * do not block acl. Otherwise block system.posix_acl_access and + * system.posix_acl_default xattrs. + */ + if (lo->user_posix_acl) { + return false; + } + if (!strcmp(name, "system.posix_acl_access") || + !strcmp(name, "system.posix_acl_default")) + return true; + + return false; +} + +/* + * Returns number of bytes in xattr_list after filtering on success. This + * could be zero as well if nothing is left after filtering. + * + * Returns negative error code on failure. + * xattr_list is modified in place. + */ +static int remove_blocked_xattrs(struct lo_data *lo, char *xattr_list, + unsigned in_size) +{ + size_t out_index, in_index; + + /* + * As of now we only filter out acl xattrs. If acls are enabled or + * they have not been explicitly disabled, there is nothing to + * filter. + */ + if (lo->user_posix_acl) { + return in_size; + } + + out_index = 0; + in_index = 0; + while (in_index < in_size) { + char *in_ptr = xattr_list + in_index; + + /* Length of current attribute name */ + size_t in_len = strlen(xattr_list + in_index) + 1; + + if (!block_xattr(lo, in_ptr)) { + if (in_index != out_index) { + memmove(xattr_list + out_index, xattr_list + in_index, in_len); + } + out_index += in_len; + } + in_index += in_len; + } + return out_index; +} + static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, size_t size) { @@ -2635,6 +2710,11 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, int saverr; int fd = -1; + if (block_xattr(lo, in_name)) { + fuse_reply_err(req, EOPNOTSUPP); + return; + } + mapped_name = NULL; name = in_name; if (lo->xattrmap) { @@ -2776,7 +2856,6 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size) if (ret == 0) { goto out; } - if (lo->xattr_map_list) { /* * Map the names back, some attributes might be dropped, @@ -2823,6 +2902,12 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size) goto out; } } + + ret = remove_blocked_xattrs(lo, value, ret); + if (ret <= 0) { + saverr = -ret; + goto out; + } fuse_reply_buf(req, value, ret); } else { /* @@ -2861,6 +2946,11 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, int saverr; int fd = -1; + if (block_xattr(lo, in_name)) { + fuse_reply_err(req, EOPNOTSUPP); + return; + } + mapped_name = NULL; name = in_name; if (lo->xattrmap) { @@ -2927,6 +3017,11 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *in_name) int saverr; int fd = -1; + if (block_xattr(lo, in_name)) { + fuse_reply_err(req, EOPNOTSUPP); + return; + } + mapped_name = NULL; name = in_name; if (lo->xattrmap) { @@ -3614,6 +3709,7 @@ int main(int argc, char *argv[]) .allow_direct_io = 0, .proc_self_fd = -1, .user_killpriv_v2 = -1, + .user_posix_acl = -1, }; struct lo_map_elem *root_elem; struct lo_map_elem *reserve_elem; @@ -3740,6 +3836,11 @@ int main(int argc, char *argv[]) exit(1); } + if (lo.user_posix_acl == 1 && !lo.xattr) { + fuse_log(FUSE_LOG_ERR, "Can't enable posix ACLs. xattrs are disabled.\n"); + exit(1); + } + lo.use_statx = true; se = fuse_session_new(&args, &lo_oper, sizeof(lo_oper), &lo); -- 2.25.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Virtio-fs] [PATCH v4 3/3] virtiofsd: Add an option to enable/disable posix acls @ 2021-02-24 16:58 ` Vivek Goyal 0 siblings, 0 replies; 8+ messages in thread From: Vivek Goyal @ 2021-02-24 16:58 UTC (permalink / raw) To: qemu-devel, virtio-fs; +Cc: vgoyal, miklos fuse has an option FUSE_POSIX_ACL which needs to be opted in by fuse server to enable posix acls. As of now we are not opting in for this, so posix acls are disabled on virtiofs by default. Add virtiofsd option "-o posix_acl/no_posix_acl" to let users enable/disable posix acl support. By default it is disabled as of now due to performance concerns with cache=none. Currently even if file server has not opted in for FUSE_POSIX_ACL, user can still query acl and set acl, and system.posix_acl_access and system.posix_acl_default xattrs show up listxattr response. Miklos said this is confusing. So he said lets block and filter system.posix_acl_access and system.posix_acl_default xattrs in getxattr/setxattr/listxattr if user has explicitly disabled posix acls using -o no_posix_acl. As of now continuing to keeping the existing behavior if user did not specify any option to disable acl support due to concerns about backward compatibility. Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- docs/tools/virtiofsd.rst | 3 + tools/virtiofsd/helper.c | 1 + tools/virtiofsd/passthrough_ll.c | 103 ++++++++++++++++++++++++++++++- 3 files changed, 106 insertions(+), 1 deletion(-) diff --git a/docs/tools/virtiofsd.rst b/docs/tools/virtiofsd.rst index 866b7db3ee..55f3bb8753 100644 --- a/docs/tools/virtiofsd.rst +++ b/docs/tools/virtiofsd.rst @@ -101,6 +101,9 @@ Options Enable/disable extended attributes (xattr) on files and directories. The default is ``no_xattr``. + * posix_acl|no_posix_acl - + Enable/disable posix acl support. Posix ACLs are disabled by default`. + .. option:: --socket-path=PATH Listen on vhost-user UNIX domain socket at PATH. diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c index 28243b51b2..800a1a6801 100644 --- a/tools/virtiofsd/helper.c +++ b/tools/virtiofsd/helper.c @@ -183,6 +183,7 @@ void fuse_cmdline_help(void) " to virtiofsd from guest applications.\n" " default: no_allow_direct_io\n" " -o announce_submounts Announce sub-mount points to the guest\n" + " -o posix_acl/no_posix_acl Enable/Disable posix_acl. (default: disabled)\n" ); } diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c index 050142e568..7931b8af32 100644 --- a/tools/virtiofsd/passthrough_ll.c +++ b/tools/virtiofsd/passthrough_ll.c @@ -172,6 +172,7 @@ struct lo_data { int user_killpriv_v2, killpriv_v2; /* If set, virtiofsd is responsible for setting umask during creation */ bool change_umask; + int user_posix_acl; }; static const struct fuse_opt lo_opts[] = { @@ -204,6 +205,8 @@ static const struct fuse_opt lo_opts[] = { { "announce_submounts", offsetof(struct lo_data, announce_submounts), 1 }, { "killpriv_v2", offsetof(struct lo_data, user_killpriv_v2), 1 }, { "no_killpriv_v2", offsetof(struct lo_data, user_killpriv_v2), 0 }, + { "posix_acl", offsetof(struct lo_data, user_posix_acl), 1 }, + { "no_posix_acl", offsetof(struct lo_data, user_posix_acl), 0 }, FUSE_OPT_END }; static bool use_syslog = false; @@ -664,6 +667,21 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn) conn->want &= ~FUSE_CAP_HANDLE_KILLPRIV_V2; lo->killpriv_v2 = 0; } + + if (lo->user_posix_acl == 1) { + /* + * User explicitly asked for this option. Enable it unconditionally. + * If connection does not have this capability, it should fail + * in fuse_lowlevel.c + */ + fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling posix acl\n"); + conn->want |= FUSE_CAP_POSIX_ACL | FUSE_CAP_DONT_MASK; + lo->change_umask = true; + } else { + /* User either did not specify anything or wants it disabled */ + fuse_log(FUSE_LOG_DEBUG, "lo_init: disabling posix_acl\n"); + conn->want &= ~FUSE_CAP_POSIX_ACL; + } } static void lo_getattr(fuse_req_t req, fuse_ino_t ino, @@ -2622,6 +2640,63 @@ static int xattr_map_server(const struct lo_data *lo, const char *server_name, return -ENODATA; } +static bool block_xattr(struct lo_data *lo, const char *name) +{ + /* + * If user explicitly enabled posix_acl or did not provide any option, + * do not block acl. Otherwise block system.posix_acl_access and + * system.posix_acl_default xattrs. + */ + if (lo->user_posix_acl) { + return false; + } + if (!strcmp(name, "system.posix_acl_access") || + !strcmp(name, "system.posix_acl_default")) + return true; + + return false; +} + +/* + * Returns number of bytes in xattr_list after filtering on success. This + * could be zero as well if nothing is left after filtering. + * + * Returns negative error code on failure. + * xattr_list is modified in place. + */ +static int remove_blocked_xattrs(struct lo_data *lo, char *xattr_list, + unsigned in_size) +{ + size_t out_index, in_index; + + /* + * As of now we only filter out acl xattrs. If acls are enabled or + * they have not been explicitly disabled, there is nothing to + * filter. + */ + if (lo->user_posix_acl) { + return in_size; + } + + out_index = 0; + in_index = 0; + while (in_index < in_size) { + char *in_ptr = xattr_list + in_index; + + /* Length of current attribute name */ + size_t in_len = strlen(xattr_list + in_index) + 1; + + if (!block_xattr(lo, in_ptr)) { + if (in_index != out_index) { + memmove(xattr_list + out_index, xattr_list + in_index, in_len); + } + out_index += in_len; + } + in_index += in_len; + } + return out_index; +} + static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, size_t size) { @@ -2635,6 +2710,11 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, int saverr; int fd = -1; + if (block_xattr(lo, in_name)) { + fuse_reply_err(req, EOPNOTSUPP); + return; + } + mapped_name = NULL; name = in_name; if (lo->xattrmap) { @@ -2776,7 +2856,6 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size) if (ret == 0) { goto out; } - if (lo->xattr_map_list) { /* * Map the names back, some attributes might be dropped, @@ -2823,6 +2902,12 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size) goto out; } } + + ret = remove_blocked_xattrs(lo, value, ret); + if (ret <= 0) { + saverr = -ret; + goto out; + } fuse_reply_buf(req, value, ret); } else { /* @@ -2861,6 +2946,11 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, int saverr; int fd = -1; + if (block_xattr(lo, in_name)) { + fuse_reply_err(req, EOPNOTSUPP); + return; + } + mapped_name = NULL; name = in_name; if (lo->xattrmap) { @@ -2927,6 +3017,11 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *in_name) int saverr; int fd = -1; + if (block_xattr(lo, in_name)) { + fuse_reply_err(req, EOPNOTSUPP); + return; + } + mapped_name = NULL; name = in_name; if (lo->xattrmap) { @@ -3614,6 +3709,7 @@ int main(int argc, char *argv[]) .allow_direct_io = 0, .proc_self_fd = -1, .user_killpriv_v2 = -1, + .user_posix_acl = -1, }; struct lo_map_elem *root_elem; struct lo_map_elem *reserve_elem; @@ -3740,6 +3836,11 @@ int main(int argc, char *argv[]) exit(1); } + if (lo.user_posix_acl == 1 && !lo.xattr) { + fuse_log(FUSE_LOG_ERR, "Can't enable posix ACLs. xattrs are disabled.\n"); + exit(1); + } + lo.use_statx = true; se = fuse_session_new(&args, &lo_oper, sizeof(lo_oper), &lo); -- 2.25.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-02-24 17:18 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-24 16:58 [PATCH v4 0/3] virtiofsd: Add support to enable/disable posix acls Vivek Goyal 2021-02-24 16:58 ` [Virtio-fs] " Vivek Goyal 2021-02-24 16:58 ` [PATCH v4 1/3] virtiofsd: Add umask to seccom allow list Vivek Goyal 2021-02-24 16:58 ` [Virtio-fs] " Vivek Goyal 2021-02-24 16:58 ` [PATCH v4 2/3] virtiofsd: Add capability to change/restore umask Vivek Goyal 2021-02-24 16:58 ` [Virtio-fs] " Vivek Goyal 2021-02-24 16:58 ` [PATCH v4 3/3] virtiofsd: Add an option to enable/disable posix acls Vivek Goyal 2021-02-24 16:58 ` [Virtio-fs] " Vivek Goyal
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.