[PATCH 0/3] virtiofsd: Add options to enable/disable posix acl

[PATCH 0/3] virtiofsd: Add options to enable/disable posix acl

[Vivek Goyal]

Luis Henriques reported that fstest generic/099 fails with virtiofs.
Little debugging showed that we don't enable acl support. So this
patch series provides option to enable/disable posix acl support. By
default it is disabled.

I have run blogbench and pjdfstests with posix acl enabled and
things work fine. 

Luis, can you please apply these patches, and run virtiofsd with
"-o posix_acl" and see if it fixes the failure you are seeing. I
ran the steps you provided manually and it fixes the issue for
me.

Vivek Goyal (3):
  virtiofsd: Add an option to enable/disable posix acls
  virtiofsd: Add umask to seccom allow list
  virtiofsd: Change umask if posix acls are enabled

 tools/virtiofsd/passthrough_ll.c      | 45 +++++++++++++++++++++++----
 tools/virtiofsd/passthrough_seccomp.c |  1 +
 2 files changed, 40 insertions(+), 6 deletions(-)

-- 
2.25.4

[Virtio-fs] [PATCH 0/3] virtiofsd: Add options to enable/disable posix acl

[Vivek Goyal]

Luis Henriques reported that fstest generic/099 fails with virtiofs.
Little debugging showed that we don't enable acl support. So this
patch series provides option to enable/disable posix acl support. By
default it is disabled.

I have run blogbench and pjdfstests with posix acl enabled and
things work fine. 

Luis, can you please apply these patches, and run virtiofsd with
"-o posix_acl" and see if it fixes the failure you are seeing. I
ran the steps you provided manually and it fixes the issue for
me.

Vivek Goyal (3):
  virtiofsd: Add an option to enable/disable posix acls
  virtiofsd: Add umask to seccom allow list
  virtiofsd: Change umask if posix acls are enabled

 tools/virtiofsd/passthrough_ll.c      | 45 +++++++++++++++++++++++----
 tools/virtiofsd/passthrough_seccomp.c |  1 +
 2 files changed, 40 insertions(+), 6 deletions(-)

-- 
2.25.4

[PATCH 1/3] virtiofsd: Add an option to enable/disable posix acls

[Vivek Goyal]

fuse has an option FUSE_POSIX_ACL which needs to be opted in by fuse
server to enable posix acls.

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.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 147b59338a..34b2848e61 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -168,6 +168,7 @@ struct lo_data {
 
     /* An O_PATH file descriptor to /proc/self/fd/ */
     int proc_self_fd;
+    int user_posix_acl;
 };
 
 static const struct fuse_opt lo_opts[] = {
@@ -198,6 +199,8 @@ static const struct fuse_opt lo_opts[] = {
     { "allow_direct_io", offsetof(struct lo_data, allow_direct_io), 1 },
     { "no_allow_direct_io", offsetof(struct lo_data, allow_direct_io), 0 },
     { "announce_submounts", offsetof(struct lo_data, announce_submounts), 1 },
+    { "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;
@@ -630,6 +633,23 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn)
                  "does not support it\n");
         lo->announce_submounts = false;
     }
+
+    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;
+    } else {
+        /*
+         * Either user specified to disable posix_acl, or did not specify
+          * anything. In both the cases do not enable posix acl.
+         */
+        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,
@@ -3533,6 +3553,7 @@ int main(int argc, char *argv[])
         .posix_lock = 0,
         .allow_direct_io = 0,
         .proc_self_fd = -1,
+        .user_posix_acl = -1,
     };
     struct lo_map_elem *root_elem;
     struct lo_map_elem *reserve_elem;
-- 
2.25.4

[Virtio-fs] [PATCH 1/3] virtiofsd: Add an option to enable/disable posix acls

[Vivek Goyal]

fuse has an option FUSE_POSIX_ACL which needs to be opted in by fuse
server to enable posix acls.

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.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 147b59338a..34b2848e61 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -168,6 +168,7 @@ struct lo_data {
 
     /* An O_PATH file descriptor to /proc/self/fd/ */
     int proc_self_fd;
+    int user_posix_acl;
 };
 
 static const struct fuse_opt lo_opts[] = {
@@ -198,6 +199,8 @@ static const struct fuse_opt lo_opts[] = {
     { "allow_direct_io", offsetof(struct lo_data, allow_direct_io), 1 },
     { "no_allow_direct_io", offsetof(struct lo_data, allow_direct_io), 0 },
     { "announce_submounts", offsetof(struct lo_data, announce_submounts), 1 },
+    { "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;
@@ -630,6 +633,23 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn)
                  "does not support it\n");
         lo->announce_submounts = false;
     }
+
+    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;
+    } else {
+        /*
+         * Either user specified to disable posix_acl, or did not specify
+          * anything. In both the cases do not enable posix acl.
+         */
+        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,
@@ -3533,6 +3553,7 @@ int main(int argc, char *argv[])
         .posix_lock = 0,
         .allow_direct_io = 0,
         .proc_self_fd = -1,
+        .user_posix_acl = -1,
     };
     struct lo_map_elem *root_elem;
     struct lo_map_elem *reserve_elem;
-- 
2.25.4

[PATCH 2/3] virtiofsd: Add umask to seccom allow list

[Vivek Goyal]

Next patch is going to make use of "umask" syscall. So allow it.

Signed-off-by: Vivek Goyal <vgoyal@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 ea852e2e33..f0313c5ce4 100644
--- a/tools/virtiofsd/passthrough_seccomp.c
+++ b/tools/virtiofsd/passthrough_seccomp.c
@@ -114,6 +114,7 @@ static const int syscall_whitelist[] = {
     SCMP_SYS(utimensat),
     SCMP_SYS(write),
     SCMP_SYS(writev),
+    SCMP_SYS(umask),
 };
 
 /* Syscalls used when --syslog is enabled */
-- 
2.25.4

[Virtio-fs] [PATCH 2/3] virtiofsd: Add umask to seccom allow list

[Vivek Goyal]

Next patch is going to make use of "umask" syscall. So allow it.

Signed-off-by: Vivek Goyal <vgoyal@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 ea852e2e33..f0313c5ce4 100644
--- a/tools/virtiofsd/passthrough_seccomp.c
+++ b/tools/virtiofsd/passthrough_seccomp.c
@@ -114,6 +114,7 @@ static const int syscall_whitelist[] = {
     SCMP_SYS(utimensat),
     SCMP_SYS(write),
     SCMP_SYS(writev),
+    SCMP_SYS(umask),
 };
 
 /* Syscalls used when --syslog is enabled */
-- 
2.25.4

[PATCH 3/3] virtiofsd: Change umask if posix acls are enabled

[Vivek Goyal]

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.

So this patch opts in for FUSE_DONT_MASK if posix acls are enabled
and changes umask to caller umask before file creation and restores
original umask after file creation is done.

This should fix fstest generic/099.

Reported-by: Luis Henriques <lhenriques@suse.de>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 34b2848e61..84691571d2 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_posix_acl;
+    /* If set, virtiofsd is responsible for setting umask during creation */
+    bool change_umask;
 };
 
 static const struct fuse_opt lo_opts[] = {
@@ -641,7 +644,8 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn)
          * in fuse_lowlevel.c
          */
         fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling posix acl\n");
-        conn->want |= FUSE_CAP_POSIX_ACL;
+        conn->want |= FUSE_CAP_POSIX_ACL | FUSE_CAP_DONT_MASK;
+        lo->change_umask = true;
     } else {
         /*
          * Either user specified to disable posix_acl, or did not specify
@@ -649,6 +653,7 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn)
          */
         fuse_log(FUSE_LOG_DEBUG, "lo_init: disabling posix_acl\n");
         conn->want &= ~FUSE_CAP_POSIX_ACL;
+        lo->change_umask = false;
     }
 }
 
@@ -1043,7 +1048,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;
 
@@ -1063,11 +1069,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;
 
@@ -1082,6 +1091,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,
@@ -1106,7 +1118,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;
     }
@@ -1115,7 +1127,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;
@@ -1780,7 +1792,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;
     }
@@ -1791,7 +1803,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

[Virtio-fs] [PATCH 3/3] virtiofsd: Change umask if posix acls are enabled

[Vivek Goyal]

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.

So this patch opts in for FUSE_DONT_MASK if posix acls are enabled
and changes umask to caller umask before file creation and restores
original umask after file creation is done.

This should fix fstest generic/099.

Reported-by: Luis Henriques <lhenriques@suse.de>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 34b2848e61..84691571d2 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_posix_acl;
+    /* If set, virtiofsd is responsible for setting umask during creation */
+    bool change_umask;
 };
 
 static const struct fuse_opt lo_opts[] = {
@@ -641,7 +644,8 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn)
          * in fuse_lowlevel.c
          */
         fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling posix acl\n");
-        conn->want |= FUSE_CAP_POSIX_ACL;
+        conn->want |= FUSE_CAP_POSIX_ACL | FUSE_CAP_DONT_MASK;
+        lo->change_umask = true;
     } else {
         /*
          * Either user specified to disable posix_acl, or did not specify
@@ -649,6 +653,7 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn)
          */
         fuse_log(FUSE_LOG_DEBUG, "lo_init: disabling posix_acl\n");
         conn->want &= ~FUSE_CAP_POSIX_ACL;
+        lo->change_umask = false;
     }
 }
 
@@ -1043,7 +1048,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;
 
@@ -1063,11 +1069,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;
 
@@ -1082,6 +1091,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,
@@ -1106,7 +1118,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;
     }
@@ -1115,7 +1127,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;
@@ -1780,7 +1792,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;
     }
@@ -1791,7 +1803,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

Re: [PATCH 0/3] virtiofsd: Add options to enable/disable posix acl

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210216233611.33400-1-vgoyal@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210216233611.33400-1-vgoyal@redhat.com
Subject: [PATCH 0/3] virtiofsd: Add options to enable/disable posix acl

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20210216233611.33400-1-vgoyal@redhat.com -> patchew/20210216233611.33400-1-vgoyal@redhat.com
Switched to a new branch 'test'
599c357 virtiofsd: Change umask if posix acls are enabled
599cc19 virtiofsd: Add umask to seccom allow list
72ea185 virtiofsd: Add an option to enable/disable posix acls

=== OUTPUT BEGIN ===
1/3 Checking commit 72ea185ac754 (virtiofsd: Add an option to enable/disable posix acls)
WARNING: Block comments should align the * on each line
#56: FILE: tools/virtiofsd/passthrough_ll.c:648:
+         * Either user specified to disable posix_acl, or did not specify
+          * anything. In both the cases do not enable posix acl.

WARNING: Block comments should align the * on each line
#57: FILE: tools/virtiofsd/passthrough_ll.c:649:
+          * anything. In both the cases do not enable posix acl.
+         */

total: 0 errors, 2 warnings, 45 lines checked

Patch 1/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/3 Checking commit 599cc19faf56 (virtiofsd: Add umask to seccom allow list)
3/3 Checking commit 599c3575ec83 (virtiofsd: Change umask if posix acls are enabled)
ERROR: braces {} are necessary for all arms of this statement
#95: FILE: tools/virtiofsd/passthrough_ll.c:1072:
+    if (change_umask)
[...]

total: 1 errors, 0 warnings, 96 lines checked

Patch 3/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210216233611.33400-1-vgoyal@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Virtio-fs] [PATCH 0/3] virtiofsd: Add options to enable/disable posix acl

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210216233611.33400-1-vgoyal@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210216233611.33400-1-vgoyal@redhat.com
Subject: [PATCH 0/3] virtiofsd: Add options to enable/disable posix acl

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
>From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20210216233611.33400-1-vgoyal@redhat.com -> patchew/20210216233611.33400-1-vgoyal@redhat.com
Switched to a new branch 'test'
599c357 virtiofsd: Change umask if posix acls are enabled
599cc19 virtiofsd: Add umask to seccom allow list
72ea185 virtiofsd: Add an option to enable/disable posix acls

=== OUTPUT BEGIN ===
1/3 Checking commit 72ea185ac754 (virtiofsd: Add an option to enable/disable posix acls)
WARNING: Block comments should align the * on each line
#56: FILE: tools/virtiofsd/passthrough_ll.c:648:
+         * Either user specified to disable posix_acl, or did not specify
+          * anything. In both the cases do not enable posix acl.

WARNING: Block comments should align the * on each line
#57: FILE: tools/virtiofsd/passthrough_ll.c:649:
+          * anything. In both the cases do not enable posix acl.
+         */

total: 0 errors, 2 warnings, 45 lines checked

Patch 1/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/3 Checking commit 599cc19faf56 (virtiofsd: Add umask to seccom allow list)
3/3 Checking commit 599c3575ec83 (virtiofsd: Change umask if posix acls are enabled)
ERROR: braces {} are necessary for all arms of this statement
#95: FILE: tools/virtiofsd/passthrough_ll.c:1072:
+    if (change_umask)
[...]

total: 1 errors, 0 warnings, 96 lines checked

Patch 3/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210216233611.33400-1-vgoyal@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 1/3] virtiofsd: Add an option to enable/disable posix acls

[Miklos Szeredi]

On Wed, Feb 17, 2021 at 12:36 AM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> fuse has an option FUSE_POSIX_ACL which needs to be opted in by fuse
> server to enable posix acls.
>
> 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.

If I read the code correctly, then no_posix_acl will still result in
system.posix_acl_* xattr ops being passed through to virtiofsd, which
will forward them to the underlying fs, resulting in posix acls
appearing to work, but doing so incorrectly (i.e. no change from
previous behavior).   Possibly better would be to have three different
modes of operation:

1) no option: default fall back to broken acl support for backward
compat (this could be removed in the future)
2) no_posix_acl: really disable acl support
3) posix_acl: enable proper acl support

Thanks,
Miklos

Re: [Virtio-fs] [PATCH 1/3] virtiofsd: Add an option to enable/disable posix acls

[Miklos Szeredi]

On Wed, Feb 17, 2021 at 12:36 AM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> fuse has an option FUSE_POSIX_ACL which needs to be opted in by fuse
> server to enable posix acls.
>
> 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.

If I read the code correctly, then no_posix_acl will still result in
system.posix_acl_* xattr ops being passed through to virtiofsd, which
will forward them to the underlying fs, resulting in posix acls
appearing to work, but doing so incorrectly (i.e. no change from
previous behavior).   Possibly better would be to have three different
modes of operation:

1) no option: default fall back to broken acl support for backward
compat (this could be removed in the future)
2) no_posix_acl: really disable acl support
3) posix_acl: enable proper acl support

Thanks,
Miklos

Re: [PATCH 0/3] virtiofsd: Add options to enable/disable posix acl

[Luis Henriques]

Vivek Goyal <vgoyal@redhat.com> writes:

> Luis Henriques reported that fstest generic/099 fails with virtiofs.
> Little debugging showed that we don't enable acl support. So this
> patch series provides option to enable/disable posix acl support. By
> default it is disabled.
>
> I have run blogbench and pjdfstests with posix acl enabled and
> things work fine. 
>
> Luis, can you please apply these patches, and run virtiofsd with
> "-o posix_acl" and see if it fixes the failure you are seeing. I
> ran the steps you provided manually and it fixes the issue for
> me.

Awesome, thanks for looking into this.  I'll have a look and test these
patches.  I expect to be able to report back later today.

Cheers,
-- 
Luis


>
> Vivek Goyal (3):
>   virtiofsd: Add an option to enable/disable posix acls
>   virtiofsd: Add umask to seccom allow list
>   virtiofsd: Change umask if posix acls are enabled
>
>  tools/virtiofsd/passthrough_ll.c      | 45 +++++++++++++++++++++++----
>  tools/virtiofsd/passthrough_seccomp.c |  1 +
>  2 files changed, 40 insertions(+), 6 deletions(-)
>
> -- 
> 2.25.4
>

Re: [Virtio-fs] [PATCH 0/3] virtiofsd: Add options to enable/disable posix acl

[Luis Henriques]

Vivek Goyal <vgoyal@redhat.com> writes:

> Luis Henriques reported that fstest generic/099 fails with virtiofs.
> Little debugging showed that we don't enable acl support. So this
> patch series provides option to enable/disable posix acl support. By
> default it is disabled.
>
> I have run blogbench and pjdfstests with posix acl enabled and
> things work fine. 
>
> Luis, can you please apply these patches, and run virtiofsd with
> "-o posix_acl" and see if it fixes the failure you are seeing. I
> ran the steps you provided manually and it fixes the issue for
> me.

Awesome, thanks for looking into this.  I'll have a look and test these
patches.  I expect to be able to report back later today.

Cheers,
-- 
Luis


>
> Vivek Goyal (3):
>   virtiofsd: Add an option to enable/disable posix acls
>   virtiofsd: Add umask to seccom allow list
>   virtiofsd: Change umask if posix acls are enabled
>
>  tools/virtiofsd/passthrough_ll.c      | 45 +++++++++++++++++++++++----
>  tools/virtiofsd/passthrough_seccomp.c |  1 +
>  2 files changed, 40 insertions(+), 6 deletions(-)
>
> -- 
> 2.25.4
>

Re: [PATCH 1/3] virtiofsd: Add an option to enable/disable posix acls

[Vivek Goyal]

On Wed, Feb 17, 2021 at 09:53:04AM +0100, Miklos Szeredi wrote:
> On Wed, Feb 17, 2021 at 12:36 AM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > fuse has an option FUSE_POSIX_ACL which needs to be opted in by fuse
> > server to enable posix acls.
> >
> > 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.
> 
> If I read the code correctly, then no_posix_acl will still result in
> system.posix_acl_* xattr ops being passed through to virtiofsd, which
> will forward them to the underlying fs, resulting in posix acls
> appearing to work, but doing so incorrectly (i.e. no change from
> previous behavior).

Yes, and this is confuing me a lot. fuse server has not indicated
support for POSIX_ACL, still user can get and set ACLs. fuse_xattr_get()
and fuse_xattr_set() must be kicking in.

I do see that we have fuse_no_acl_xattr_handlers and that should
be able to block setting/getting acls if acl support is not there
but we register it only if we are not mounted in init_user_ns.

        if (sb->s_user_ns != &init_user_ns)
                sb->s_xattr = fuse_no_acl_xattr_handlers;

So question is, should fuse client be fixed as well to block setting
and getting acls if fuse server does not support ACL? Or we now need
to keep it around for backward compatibility.

> Possibly better would be to have three different
> modes of operation:
> 
> 1) no option: default fall back to broken acl support for backward
> compat (this could be removed in the future)

What about FUSE_DONT_MASK in this mode. ACLs are not enabled but
user can get/set these. Should that mean we still honor default
acl and not apply umask?

Probably I should opt for FUSE_DONT_MASK only if posix_acl support is
enabled. Given this does not work even today (atleast for virtiofs), so
it is not a backward compatibility issue. And its confusing anyway.

> 2) no_posix_acl: really disable acl support

That is block getting and setting system.posix_acl xattr. Will do that.
I think we will have to block it even if somebody has remapped xattrs
in virtiofsd.

> 3) posix_acl: enable proper acl support

Thanks
Vivek

> 
> Thanks,
> Miklos
> 

Re: [Virtio-fs] [PATCH 1/3] virtiofsd: Add an option to enable/disable posix acls

[Vivek Goyal]

On Wed, Feb 17, 2021 at 09:53:04AM +0100, Miklos Szeredi wrote:
> On Wed, Feb 17, 2021 at 12:36 AM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > fuse has an option FUSE_POSIX_ACL which needs to be opted in by fuse
> > server to enable posix acls.
> >
> > 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.
> 
> If I read the code correctly, then no_posix_acl will still result in
> system.posix_acl_* xattr ops being passed through to virtiofsd, which
> will forward them to the underlying fs, resulting in posix acls
> appearing to work, but doing so incorrectly (i.e. no change from
> previous behavior).

Yes, and this is confuing me a lot. fuse server has not indicated
support for POSIX_ACL, still user can get and set ACLs. fuse_xattr_get()
and fuse_xattr_set() must be kicking in.

I do see that we have fuse_no_acl_xattr_handlers and that should
be able to block setting/getting acls if acl support is not there
but we register it only if we are not mounted in init_user_ns.

        if (sb->s_user_ns != &init_user_ns)
                sb->s_xattr = fuse_no_acl_xattr_handlers;

So question is, should fuse client be fixed as well to block setting
and getting acls if fuse server does not support ACL? Or we now need
to keep it around for backward compatibility.

> Possibly better would be to have three different
> modes of operation:
> 
> 1) no option: default fall back to broken acl support for backward
> compat (this could be removed in the future)

What about FUSE_DONT_MASK in this mode. ACLs are not enabled but
user can get/set these. Should that mean we still honor default
acl and not apply umask?

Probably I should opt for FUSE_DONT_MASK only if posix_acl support is
enabled. Given this does not work even today (atleast for virtiofs), so
it is not a backward compatibility issue. And its confusing anyway.

> 2) no_posix_acl: really disable acl support

That is block getting and setting system.posix_acl xattr. Will do that.
I think we will have to block it even if somebody has remapped xattrs
in virtiofsd.

> 3) posix_acl: enable proper acl support

Thanks
Vivek

> 
> Thanks,
> Miklos
> 

Re: [PATCH 1/3] virtiofsd: Add an option to enable/disable posix acls

[Miklos Szeredi]

On Wed, Feb 17, 2021 at 4:07 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Wed, Feb 17, 2021 at 09:53:04AM +0100, Miklos Szeredi wrote:
> > On Wed, Feb 17, 2021 at 12:36 AM Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > fuse has an option FUSE_POSIX_ACL which needs to be opted in by fuse
> > > server to enable posix acls.
> > >
> > > 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.
> >
> > If I read the code correctly, then no_posix_acl will still result in
> > system.posix_acl_* xattr ops being passed through to virtiofsd, which
> > will forward them to the underlying fs, resulting in posix acls
> > appearing to work, but doing so incorrectly (i.e. no change from
> > previous behavior).
>
> Yes, and this is confuing me a lot. fuse server has not indicated
> support for POSIX_ACL, still user can get and set ACLs. fuse_xattr_get()
> and fuse_xattr_set() must be kicking in.
>
> I do see that we have fuse_no_acl_xattr_handlers and that should
> be able to block setting/getting acls if acl support is not there
> but we register it only if we are not mounted in init_user_ns.
>
>         if (sb->s_user_ns != &init_user_ns)
>                 sb->s_xattr = fuse_no_acl_xattr_handlers;
>
> So question is, should fuse client be fixed as well to block setting
> and getting acls if fuse server does not support ACL? Or we now need
> to keep it around for backward compatibility.

Yes, this is a compatibility thing.   User namespaces don't work
without actual ACL ops, so this was disabled in that case and no
backward compatibility worries in that case.   We should have disabled
this for virtiofs from the start, but at this point we are again stuck
with a backward compatibility issue.

Alternatively make posix_acl the default, hence fixing the bad
behavior is unlikely to cause a regression.

>
> > Possibly better would be to have three different
> > modes of operation:
> >
> > 1) no option: default fall back to broken acl support for backward
> > compat (this could be removed in the future)
>
> What about FUSE_DONT_MASK in this mode. ACLs are not enabled but
> user can get/set these. Should that mean we still honor default
> acl and not apply umask?
>
> Probably I should opt for FUSE_DONT_MASK only if posix_acl support is
> enabled. Given this does not work even today (atleast for virtiofs), so
> it is not a backward compatibility issue. And its confusing anyway.
>
> > 2) no_posix_acl: really disable acl support
>
> That is block getting and setting system.posix_acl xattr. Will do that.
> I think we will have to block it even if somebody has remapped xattrs
> in virtiofsd.

Okay.

Thanks,
Miklos

Re: [Virtio-fs] [PATCH 1/3] virtiofsd: Add an option to enable/disable posix acls

[Miklos Szeredi]

On Wed, Feb 17, 2021 at 4:07 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Wed, Feb 17, 2021 at 09:53:04AM +0100, Miklos Szeredi wrote:
> > On Wed, Feb 17, 2021 at 12:36 AM Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > fuse has an option FUSE_POSIX_ACL which needs to be opted in by fuse
> > > server to enable posix acls.
> > >
> > > 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.
> >
> > If I read the code correctly, then no_posix_acl will still result in
> > system.posix_acl_* xattr ops being passed through to virtiofsd, which
> > will forward them to the underlying fs, resulting in posix acls
> > appearing to work, but doing so incorrectly (i.e. no change from
> > previous behavior).
>
> Yes, and this is confuing me a lot. fuse server has not indicated
> support for POSIX_ACL, still user can get and set ACLs. fuse_xattr_get()
> and fuse_xattr_set() must be kicking in.
>
> I do see that we have fuse_no_acl_xattr_handlers and that should
> be able to block setting/getting acls if acl support is not there
> but we register it only if we are not mounted in init_user_ns.
>
>         if (sb->s_user_ns != &init_user_ns)
>                 sb->s_xattr = fuse_no_acl_xattr_handlers;
>
> So question is, should fuse client be fixed as well to block setting
> and getting acls if fuse server does not support ACL? Or we now need
> to keep it around for backward compatibility.

Yes, this is a compatibility thing.   User namespaces don't work
without actual ACL ops, so this was disabled in that case and no
backward compatibility worries in that case.   We should have disabled
this for virtiofs from the start, but at this point we are again stuck
with a backward compatibility issue.

Alternatively make posix_acl the default, hence fixing the bad
behavior is unlikely to cause a regression.

>
> > Possibly better would be to have three different
> > modes of operation:
> >
> > 1) no option: default fall back to broken acl support for backward
> > compat (this could be removed in the future)
>
> What about FUSE_DONT_MASK in this mode. ACLs are not enabled but
> user can get/set these. Should that mean we still honor default
> acl and not apply umask?
>
> Probably I should opt for FUSE_DONT_MASK only if posix_acl support is
> enabled. Given this does not work even today (atleast for virtiofs), so
> it is not a backward compatibility issue. And its confusing anyway.
>
> > 2) no_posix_acl: really disable acl support
>
> That is block getting and setting system.posix_acl xattr. Will do that.
> I think we will have to block it even if somebody has remapped xattrs
> in virtiofsd.

Okay.

Thanks,
Miklos