[PATCH v7 0/7] virtiofsd: Add support to enable/disable posix acls

[PATCH v7 0/7] virtiofsd: Add support to enable/disable posix acls

[Vivek Goyal]

Hi,

This is V7 of the patches.

Changes since V6.

- Dropped kernel header update patch as somebody else did it.
- Fixed coding style issues.

Currently posix ACL support does not work well with virtiofs and bunch
of tests fail when I run xfstests "./check -g acl".

This patches series fixes the issues with virtiofs posix acl support
and provides options to enable/disable posix acl (-o posix_acl/no_posix_acl).
By default posix_acls are disabled.

With this patch series applied and virtiofsd running with "-o posix_acl",
xfstests "./check -g acl" passes.

Thanks
Vivek


Vivek Goyal (7):
  virtiofsd: Fix fuse setxattr() API change issue
  virtiofsd: Fix xattr operations overwriting errno
  virtiofsd: Add support for extended setxattr
  virtiofsd: Add umask to seccom allow list
  virtiofsd: Add capability to change/restore umask
  virtiofsd: Switch creds, drop FSETID for system.posix_acl_access xattr
  virtiofsd: Add an option to enable/disable posix acls

 docs/tools/virtiofsd.rst              |   3 +
 tools/virtiofsd/fuse_common.h         |  10 ++
 tools/virtiofsd/fuse_lowlevel.c       |  18 +-
 tools/virtiofsd/fuse_lowlevel.h       |   3 +-
 tools/virtiofsd/helper.c              |   1 +
 tools/virtiofsd/passthrough_ll.c      | 229 ++++++++++++++++++++++++--
 tools/virtiofsd/passthrough_seccomp.c |   1 +
 7 files changed, 249 insertions(+), 16 deletions(-)

-- 
2.25.4

[Virtio-fs] [PATCH v7 0/7] virtiofsd: Add support to enable/disable posix acls

[Vivek Goyal]

Hi,

This is V7 of the patches.

Changes since V6.

- Dropped kernel header update patch as somebody else did it.
- Fixed coding style issues.

Currently posix ACL support does not work well with virtiofs and bunch
of tests fail when I run xfstests "./check -g acl".

This patches series fixes the issues with virtiofs posix acl support
and provides options to enable/disable posix acl (-o posix_acl/no_posix_acl).
By default posix_acls are disabled.

With this patch series applied and virtiofsd running with "-o posix_acl",
xfstests "./check -g acl" passes.

Thanks
Vivek


Vivek Goyal (7):
  virtiofsd: Fix fuse setxattr() API change issue
  virtiofsd: Fix xattr operations overwriting errno
  virtiofsd: Add support for extended setxattr
  virtiofsd: Add umask to seccom allow list
  virtiofsd: Add capability to change/restore umask
  virtiofsd: Switch creds, drop FSETID for system.posix_acl_access xattr
  virtiofsd: Add an option to enable/disable posix acls

 docs/tools/virtiofsd.rst              |   3 +
 tools/virtiofsd/fuse_common.h         |  10 ++
 tools/virtiofsd/fuse_lowlevel.c       |  18 +-
 tools/virtiofsd/fuse_lowlevel.h       |   3 +-
 tools/virtiofsd/helper.c              |   1 +
 tools/virtiofsd/passthrough_ll.c      | 229 ++++++++++++++++++++++++--
 tools/virtiofsd/passthrough_seccomp.c |   1 +
 7 files changed, 249 insertions(+), 16 deletions(-)

-- 
2.25.4

[PATCH v7 1/7] virtiofsd: Fix fuse setxattr() API change issue

[Vivek Goyal]

With kernel header updates fuse_setxattr_in struct has grown in size.
But this new struct size only takes affect if user has opted in
for fuse feature FUSE_SETXATTR_EXT otherwise fuse continues to
send "fuse_setxattr_in" of older size. Older size is determined
by FUSE_COMPAT_SETXATTR_IN_SIZE.

Fix this. If we have not opted in for FUSE_SETXATTR_EXT, then
expect that we will get fuse_setxattr_in of size FUSE_COMPAT_SETXATTR_IN_SIZE
and not sizeof(struct fuse_sexattr_in).

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/fuse_common.h   | 5 +++++
 tools/virtiofsd/fuse_lowlevel.c | 7 ++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/tools/virtiofsd/fuse_common.h b/tools/virtiofsd/fuse_common.h
index fa9671872e..0c2665b977 100644
--- a/tools/virtiofsd/fuse_common.h
+++ b/tools/virtiofsd/fuse_common.h
@@ -372,6 +372,11 @@ struct fuse_file_info {
  */
 #define FUSE_CAP_HANDLE_KILLPRIV_V2 (1 << 28)
 
+/**
+ * Indicates that file server supports extended struct fuse_setxattr_in
+ */
+#define FUSE_CAP_SETXATTR_EXT (1 << 29)
+
 /**
  * Ioctl flags
  *
diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
index 7fe2cef1eb..c2b6ff1686 100644
--- a/tools/virtiofsd/fuse_lowlevel.c
+++ b/tools/virtiofsd/fuse_lowlevel.c
@@ -1419,8 +1419,13 @@ static void do_setxattr(fuse_req_t req, fuse_ino_t nodeid,
     struct fuse_setxattr_in *arg;
     const char *name;
     const char *value;
+    bool setxattr_ext = req->se->conn.want & FUSE_CAP_SETXATTR_EXT;
 
-    arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
+    if (setxattr_ext) {
+        arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
+    } else {
+        arg = fuse_mbuf_iter_advance(iter, FUSE_COMPAT_SETXATTR_IN_SIZE);
+    }
     name = fuse_mbuf_iter_advance_str(iter);
     if (!arg || !name) {
         fuse_reply_err(req, EINVAL);
-- 
2.25.4

[Virtio-fs] [PATCH v7 1/7] virtiofsd: Fix fuse setxattr() API change issue

[Vivek Goyal]

With kernel header updates fuse_setxattr_in struct has grown in size.
But this new struct size only takes affect if user has opted in
for fuse feature FUSE_SETXATTR_EXT otherwise fuse continues to
send "fuse_setxattr_in" of older size. Older size is determined
by FUSE_COMPAT_SETXATTR_IN_SIZE.

Fix this. If we have not opted in for FUSE_SETXATTR_EXT, then
expect that we will get fuse_setxattr_in of size FUSE_COMPAT_SETXATTR_IN_SIZE
and not sizeof(struct fuse_sexattr_in).

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/fuse_common.h   | 5 +++++
 tools/virtiofsd/fuse_lowlevel.c | 7 ++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/tools/virtiofsd/fuse_common.h b/tools/virtiofsd/fuse_common.h
index fa9671872e..0c2665b977 100644
--- a/tools/virtiofsd/fuse_common.h
+++ b/tools/virtiofsd/fuse_common.h
@@ -372,6 +372,11 @@ struct fuse_file_info {
  */
 #define FUSE_CAP_HANDLE_KILLPRIV_V2 (1 << 28)
 
+/**
+ * Indicates that file server supports extended struct fuse_setxattr_in
+ */
+#define FUSE_CAP_SETXATTR_EXT (1 << 29)
+
 /**
  * Ioctl flags
  *
diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
index 7fe2cef1eb..c2b6ff1686 100644
--- a/tools/virtiofsd/fuse_lowlevel.c
+++ b/tools/virtiofsd/fuse_lowlevel.c
@@ -1419,8 +1419,13 @@ static void do_setxattr(fuse_req_t req, fuse_ino_t nodeid,
     struct fuse_setxattr_in *arg;
     const char *name;
     const char *value;
+    bool setxattr_ext = req->se->conn.want & FUSE_CAP_SETXATTR_EXT;
 
-    arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
+    if (setxattr_ext) {
+        arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
+    } else {
+        arg = fuse_mbuf_iter_advance(iter, FUSE_COMPAT_SETXATTR_IN_SIZE);
+    }
     name = fuse_mbuf_iter_advance_str(iter);
     if (!arg || !name) {
         fuse_reply_err(req, EINVAL);
-- 
2.25.4

[PATCH v7 2/7] virtiofsd: Fix xattr operations overwriting errno

[Vivek Goyal]

getxattr/setxattr/removexattr/listxattr operations handle regualar
and non-regular files differently. For the case of non-regular files
we do fchdir(/proc/self/fd) and the xattr operation and then revert
back to original working directory. After this we are saving errno
and that's buggy because fchdir() will overwrite the errno.

FCHDIR_NOFAIL(lo->proc_self_fd);
ret = getxattr(procname, name, value, size);
FCHDIR_NOFAIL(lo->root.fd);

if (ret == -1)
    saverr = errno

In above example, if getxattr() failed, we will still return 0 to caller
as errno must have been written by FCHDIR_NOFAIL(lo->root.fd) call.
Fix all such instances and capture "errno" early and save in "saverr"
variable.

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

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 49c21fd855..ec91b3c133 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -2791,15 +2791,17 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
             goto out_err;
         }
         ret = fgetxattr(fd, name, value, size);
+        saverr = ret == -1 ? errno : 0;
     } else {
         /* fchdir should not fail here */
         FCHDIR_NOFAIL(lo->proc_self_fd);
         ret = getxattr(procname, name, value, size);
+        saverr = ret == -1 ? errno : 0;
         FCHDIR_NOFAIL(lo->root.fd);
     }
 
     if (ret == -1) {
-        goto out_err;
+        goto out;
     }
     if (size) {
         saverr = 0;
@@ -2864,15 +2866,17 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
             goto out_err;
         }
         ret = flistxattr(fd, value, size);
+        saverr = ret == -1 ? errno : 0;
     } else {
         /* fchdir should not fail here */
         FCHDIR_NOFAIL(lo->proc_self_fd);
         ret = listxattr(procname, value, size);
+        saverr = ret == -1 ? errno : 0;
         FCHDIR_NOFAIL(lo->root.fd);
     }
 
     if (ret == -1) {
-        goto out_err;
+        goto out;
     }
     if (size) {
         saverr = 0;
@@ -2998,15 +3002,15 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
             goto out;
         }
         ret = fsetxattr(fd, name, value, size, flags);
+        saverr = ret == -1 ? errno : 0;
     } else {
         /* fchdir should not fail here */
         FCHDIR_NOFAIL(lo->proc_self_fd);
         ret = setxattr(procname, name, value, size, flags);
+        saverr = ret == -1 ? errno : 0;
         FCHDIR_NOFAIL(lo->root.fd);
     }
 
-    saverr = ret == -1 ? errno : 0;
-
 out:
     if (fd >= 0) {
         close(fd);
@@ -3064,15 +3068,15 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *in_name)
             goto out;
         }
         ret = fremovexattr(fd, name);
+        saverr = ret == -1 ? errno : 0;
     } else {
         /* fchdir should not fail here */
         FCHDIR_NOFAIL(lo->proc_self_fd);
         ret = removexattr(procname, name);
+        saverr = ret == -1 ? errno : 0;
         FCHDIR_NOFAIL(lo->root.fd);
     }
 
-    saverr = ret == -1 ? errno : 0;
-
 out:
     if (fd >= 0) {
         close(fd);
-- 
2.25.4

[Virtio-fs] [PATCH v7 2/7] virtiofsd: Fix xattr operations overwriting errno

[Vivek Goyal]

getxattr/setxattr/removexattr/listxattr operations handle regualar
and non-regular files differently. For the case of non-regular files
we do fchdir(/proc/self/fd) and the xattr operation and then revert
back to original working directory. After this we are saving errno
and that's buggy because fchdir() will overwrite the errno.

FCHDIR_NOFAIL(lo->proc_self_fd);
ret = getxattr(procname, name, value, size);
FCHDIR_NOFAIL(lo->root.fd);

if (ret == -1)
    saverr = errno

In above example, if getxattr() failed, we will still return 0 to caller
as errno must have been written by FCHDIR_NOFAIL(lo->root.fd) call.
Fix all such instances and capture "errno" early and save in "saverr"
variable.

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

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 49c21fd855..ec91b3c133 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -2791,15 +2791,17 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
             goto out_err;
         }
         ret = fgetxattr(fd, name, value, size);
+        saverr = ret == -1 ? errno : 0;
     } else {
         /* fchdir should not fail here */
         FCHDIR_NOFAIL(lo->proc_self_fd);
         ret = getxattr(procname, name, value, size);
+        saverr = ret == -1 ? errno : 0;
         FCHDIR_NOFAIL(lo->root.fd);
     }
 
     if (ret == -1) {
-        goto out_err;
+        goto out;
     }
     if (size) {
         saverr = 0;
@@ -2864,15 +2866,17 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
             goto out_err;
         }
         ret = flistxattr(fd, value, size);
+        saverr = ret == -1 ? errno : 0;
     } else {
         /* fchdir should not fail here */
         FCHDIR_NOFAIL(lo->proc_self_fd);
         ret = listxattr(procname, value, size);
+        saverr = ret == -1 ? errno : 0;
         FCHDIR_NOFAIL(lo->root.fd);
     }
 
     if (ret == -1) {
-        goto out_err;
+        goto out;
     }
     if (size) {
         saverr = 0;
@@ -2998,15 +3002,15 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
             goto out;
         }
         ret = fsetxattr(fd, name, value, size, flags);
+        saverr = ret == -1 ? errno : 0;
     } else {
         /* fchdir should not fail here */
         FCHDIR_NOFAIL(lo->proc_self_fd);
         ret = setxattr(procname, name, value, size, flags);
+        saverr = ret == -1 ? errno : 0;
         FCHDIR_NOFAIL(lo->root.fd);
     }
 
-    saverr = ret == -1 ? errno : 0;
-
 out:
     if (fd >= 0) {
         close(fd);
@@ -3064,15 +3068,15 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *in_name)
             goto out;
         }
         ret = fremovexattr(fd, name);
+        saverr = ret == -1 ? errno : 0;
     } else {
         /* fchdir should not fail here */
         FCHDIR_NOFAIL(lo->proc_self_fd);
         ret = removexattr(procname, name);
+        saverr = ret == -1 ? errno : 0;
         FCHDIR_NOFAIL(lo->root.fd);
     }
 
-    saverr = ret == -1 ? errno : 0;
-
 out:
     if (fd >= 0) {
         close(fd);
-- 
2.25.4

[PATCH v7 3/7] virtiofsd: Add support for extended setxattr

[Vivek Goyal]

Add the bits to enable support for setxattr_ext if fuse offers it. Do not
enable it by default yet. Let passthrough_ll opt-in. Enabling it by deafult
kind of automatically means that you are taking responsibility of clearing
SGID if ACL is set.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/fuse_common.h    |  5 +++++
 tools/virtiofsd/fuse_lowlevel.c  | 11 ++++++++++-
 tools/virtiofsd/fuse_lowlevel.h  |  3 ++-
 tools/virtiofsd/passthrough_ll.c |  3 ++-
 4 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/tools/virtiofsd/fuse_common.h b/tools/virtiofsd/fuse_common.h
index 0c2665b977..8abac80098 100644
--- a/tools/virtiofsd/fuse_common.h
+++ b/tools/virtiofsd/fuse_common.h
@@ -377,6 +377,11 @@ struct fuse_file_info {
  */
 #define FUSE_CAP_SETXATTR_EXT (1 << 29)
 
+/**
+ * Indicates that file server supports extended struct fuse_setxattr_in
+ */
+#define FUSE_CAP_SETXATTR_EXT (1 << 29)
+
 /**
  * Ioctl flags
  *
diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
index c2b6ff1686..d1e24c013f 100644
--- a/tools/virtiofsd/fuse_lowlevel.c
+++ b/tools/virtiofsd/fuse_lowlevel.c
@@ -1439,7 +1439,9 @@ static void do_setxattr(fuse_req_t req, fuse_ino_t nodeid,
     }
 
     if (req->se->op.setxattr) {
-        req->se->op.setxattr(req, nodeid, name, value, arg->size, arg->flags);
+        uint32_t setxattr_flags = setxattr_ext ? arg->setxattr_flags : 0;
+        req->se->op.setxattr(req, nodeid, name, value, arg->size, arg->flags,
+                             setxattr_flags);
     } else {
         fuse_reply_err(req, ENOSYS);
     }
@@ -1986,6 +1988,9 @@ static void do_init(fuse_req_t req, fuse_ino_t nodeid,
     if (arg->flags & FUSE_HANDLE_KILLPRIV_V2) {
         se->conn.capable |= FUSE_CAP_HANDLE_KILLPRIV_V2;
     }
+    if (arg->flags & FUSE_SETXATTR_EXT) {
+        se->conn.capable |= FUSE_CAP_SETXATTR_EXT;
+    }
 #ifdef HAVE_SPLICE
 #ifdef HAVE_VMSPLICE
     se->conn.capable |= FUSE_CAP_SPLICE_WRITE | FUSE_CAP_SPLICE_MOVE;
@@ -2121,6 +2126,10 @@ static void do_init(fuse_req_t req, fuse_ino_t nodeid,
         outarg.flags |= FUSE_HANDLE_KILLPRIV_V2;
     }
 
+    if (se->conn.want & FUSE_CAP_SETXATTR_EXT) {
+        outarg.flags |= FUSE_SETXATTR_EXT;
+    }
+
     fuse_log(FUSE_LOG_DEBUG, "   INIT: %u.%u\n", outarg.major, outarg.minor);
     fuse_log(FUSE_LOG_DEBUG, "   flags=0x%08x\n", outarg.flags);
     fuse_log(FUSE_LOG_DEBUG, "   max_readahead=0x%08x\n", outarg.max_readahead);
diff --git a/tools/virtiofsd/fuse_lowlevel.h b/tools/virtiofsd/fuse_lowlevel.h
index 3bf786b034..4b4e8c9724 100644
--- a/tools/virtiofsd/fuse_lowlevel.h
+++ b/tools/virtiofsd/fuse_lowlevel.h
@@ -798,7 +798,8 @@ struct fuse_lowlevel_ops {
      *   fuse_reply_err
      */
     void (*setxattr)(fuse_req_t req, fuse_ino_t ino, const char *name,
-                     const char *value, size_t size, int flags);
+                     const char *value, size_t size, int flags,
+                     uint32_t setxattr_flags);
 
     /**
      * Get an extended attribute
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index ec91b3c133..9f5cd98fb5 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -2955,7 +2955,8 @@ out:
 }
 
 static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
-                        const char *value, size_t size, int flags)
+                        const char *value, size_t size, int flags,
+                        uint32_t extra_flags)
 {
     char procname[64];
     const char *name;
-- 
2.25.4

[Virtio-fs] [PATCH v7 3/7] virtiofsd: Add support for extended setxattr

[Vivek Goyal]

Add the bits to enable support for setxattr_ext if fuse offers it. Do not
enable it by default yet. Let passthrough_ll opt-in. Enabling it by deafult
kind of automatically means that you are taking responsibility of clearing
SGID if ACL is set.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/fuse_common.h    |  5 +++++
 tools/virtiofsd/fuse_lowlevel.c  | 11 ++++++++++-
 tools/virtiofsd/fuse_lowlevel.h  |  3 ++-
 tools/virtiofsd/passthrough_ll.c |  3 ++-
 4 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/tools/virtiofsd/fuse_common.h b/tools/virtiofsd/fuse_common.h
index 0c2665b977..8abac80098 100644
--- a/tools/virtiofsd/fuse_common.h
+++ b/tools/virtiofsd/fuse_common.h
@@ -377,6 +377,11 @@ struct fuse_file_info {
  */
 #define FUSE_CAP_SETXATTR_EXT (1 << 29)
 
+/**
+ * Indicates that file server supports extended struct fuse_setxattr_in
+ */
+#define FUSE_CAP_SETXATTR_EXT (1 << 29)
+
 /**
  * Ioctl flags
  *
diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
index c2b6ff1686..d1e24c013f 100644
--- a/tools/virtiofsd/fuse_lowlevel.c
+++ b/tools/virtiofsd/fuse_lowlevel.c
@@ -1439,7 +1439,9 @@ static void do_setxattr(fuse_req_t req, fuse_ino_t nodeid,
     }
 
     if (req->se->op.setxattr) {
-        req->se->op.setxattr(req, nodeid, name, value, arg->size, arg->flags);
+        uint32_t setxattr_flags = setxattr_ext ? arg->setxattr_flags : 0;
+        req->se->op.setxattr(req, nodeid, name, value, arg->size, arg->flags,
+                             setxattr_flags);
     } else {
         fuse_reply_err(req, ENOSYS);
     }
@@ -1986,6 +1988,9 @@ static void do_init(fuse_req_t req, fuse_ino_t nodeid,
     if (arg->flags & FUSE_HANDLE_KILLPRIV_V2) {
         se->conn.capable |= FUSE_CAP_HANDLE_KILLPRIV_V2;
     }
+    if (arg->flags & FUSE_SETXATTR_EXT) {
+        se->conn.capable |= FUSE_CAP_SETXATTR_EXT;
+    }
 #ifdef HAVE_SPLICE
 #ifdef HAVE_VMSPLICE
     se->conn.capable |= FUSE_CAP_SPLICE_WRITE | FUSE_CAP_SPLICE_MOVE;
@@ -2121,6 +2126,10 @@ static void do_init(fuse_req_t req, fuse_ino_t nodeid,
         outarg.flags |= FUSE_HANDLE_KILLPRIV_V2;
     }
 
+    if (se->conn.want & FUSE_CAP_SETXATTR_EXT) {
+        outarg.flags |= FUSE_SETXATTR_EXT;
+    }
+
     fuse_log(FUSE_LOG_DEBUG, "   INIT: %u.%u\n", outarg.major, outarg.minor);
     fuse_log(FUSE_LOG_DEBUG, "   flags=0x%08x\n", outarg.flags);
     fuse_log(FUSE_LOG_DEBUG, "   max_readahead=0x%08x\n", outarg.max_readahead);
diff --git a/tools/virtiofsd/fuse_lowlevel.h b/tools/virtiofsd/fuse_lowlevel.h
index 3bf786b034..4b4e8c9724 100644
--- a/tools/virtiofsd/fuse_lowlevel.h
+++ b/tools/virtiofsd/fuse_lowlevel.h
@@ -798,7 +798,8 @@ struct fuse_lowlevel_ops {
      *   fuse_reply_err
      */
     void (*setxattr)(fuse_req_t req, fuse_ino_t ino, const char *name,
-                     const char *value, size_t size, int flags);
+                     const char *value, size_t size, int flags,
+                     uint32_t setxattr_flags);
 
     /**
      * Get an extended attribute
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index ec91b3c133..9f5cd98fb5 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -2955,7 +2955,8 @@ out:
 }
 
 static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
-                        const char *value, size_t size, int flags)
+                        const char *value, size_t size, int flags,
+                        uint32_t extra_flags)
 {
     char procname[64];
     const char *name;
-- 
2.25.4

[PATCH v7 4/7] virtiofsd: Add umask to seccom allow list

[Vivek Goyal]

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

[Virtio-fs] [PATCH v7 4/7] virtiofsd: Add umask to seccom allow list

[Vivek Goyal]

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

[PATCH v7 5/7] virtiofsd: Add capability to change/restore umask

[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.

This patch only adds capability to change umask and restore it. It
does not enable it yet. Next few patches 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 9f5cd98fb5..0c9084ea15 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -122,6 +122,7 @@ struct lo_inode {
 struct lo_cred {
     uid_t euid;
     gid_t egid;
+    mode_t umask;
 };
 
 enum {
@@ -172,6 +173,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[] = {
@@ -1134,7 +1137,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;
 
@@ -1154,11 +1158,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;
 
@@ -1173,6 +1180,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,
@@ -1202,7 +1212,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;
     }
@@ -1211,7 +1221,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;
@@ -1917,7 +1927,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;
     }
@@ -1928,7 +1938,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 v7 5/7] virtiofsd: Add capability to change/restore umask

[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.

This patch only adds capability to change umask and restore it. It
does not enable it yet. Next few patches 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 9f5cd98fb5..0c9084ea15 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -122,6 +122,7 @@ struct lo_inode {
 struct lo_cred {
     uid_t euid;
     gid_t egid;
+    mode_t umask;
 };
 
 enum {
@@ -172,6 +173,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[] = {
@@ -1134,7 +1137,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;
 
@@ -1154,11 +1158,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;
 
@@ -1173,6 +1180,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,
@@ -1202,7 +1212,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;
     }
@@ -1211,7 +1221,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;
@@ -1917,7 +1927,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;
     }
@@ -1928,7 +1938,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

[PATCH v7 6/7] virtiofsd: Switch creds, drop FSETID for system.posix_acl_access xattr

[Vivek Goyal]

When posix access acls are set on a file, it can lead to adjusting file
permissions (mode) as well. If caller does not have CAP_FSETID and it
also does not have membership of owner group, this will lead to clearing
SGID bit in mode.

Current fuse code is written in such a way that it expects file server
to take care of chaning file mode (permission), if there is a need.
Right now, host kernel does not clear SGID bit because virtiofsd is
running as root and has CAP_FSETID. For host kernel to clear SGID,
virtiofsd need to switch to gid of caller in guest and also drop
CAP_FSETID (if caller did not have it to begin with).

If SGID needs to be cleared, client will set the flag
FUSE_SETXATTR_ACL_KILL_SGID in setxattr request. In that case server
should kill sgid.

Currently just switch to uid/gid of the caller and drop CAP_FSETID
and that should do it.

This should fix the xfstest generic/375 test case.

We don't have to switch uid for this to work. That could be one optimization
that pass a parameter to lo_change_cred() to only switch gid and not uid.

Also this will not work whenever (if ever) we support idmapped mounts. In
that case it is possible that uid/gid in request are 0/0 but still we
need to clear SGID. So we will have to pick a non-root sgid and switch
to that instead. That's an TODO item for future when idmapped mount
support is introduced.

This patch only adds the capability to switch creds and drop FSETID
when acl xattr is set. This does not take affect yet. It can take
affect when next patch adds the capability to enable posix_acl.

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

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 0c9084ea15..113c725def 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -175,6 +175,7 @@ struct lo_data {
     int user_killpriv_v2, killpriv_v2;
     /* If set, virtiofsd is responsible for setting umask during creation */
     bool change_umask;
+    int posix_acl;
 };
 
 static const struct fuse_opt lo_opts[] = {
@@ -1185,6 +1186,51 @@ static void lo_restore_cred(struct lo_cred *old, bool restore_umask)
         umask(old->umask);
 }
 
+/*
+ * A helper to change cred and drop capability. Returns 0 on success and
+ * errno on error
+ */
+static int lo_drop_cap_change_cred(fuse_req_t req, struct lo_cred *old,
+                                   bool change_umask, const char *cap_name,
+                                   bool *cap_dropped)
+{
+    int ret;
+    bool __cap_dropped;
+
+    assert(cap_name);
+
+    ret = drop_effective_cap(cap_name, &__cap_dropped);
+    if (ret) {
+        return ret;
+    }
+
+    ret = lo_change_cred(req, old, change_umask);
+    if (ret) {
+        if (__cap_dropped) {
+            if (gain_effective_cap(cap_name)) {
+                fuse_log(FUSE_LOG_ERR, "Failed to gain CAP_%s\n", cap_name);
+            }
+        }
+    }
+
+    if (cap_dropped) {
+        *cap_dropped = __cap_dropped;
+    }
+    return ret;
+}
+
+static void lo_restore_cred_gain_cap(struct lo_cred *old, bool restore_umask,
+                                     const char *cap_name)
+{
+    assert(cap_name);
+
+    lo_restore_cred(old, restore_umask);
+
+    if (gain_effective_cap(cap_name)) {
+        fuse_log(FUSE_LOG_ERR, "Failed to gain CAP_%s\n", cap_name);
+    }
+}
+
 static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent,
                              const char *name, mode_t mode, dev_t rdev,
                              const char *link)
@@ -2976,6 +3022,9 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
     ssize_t ret;
     int saverr;
     int fd = -1;
+    bool switched_creds = false;
+    bool cap_fsetid_dropped = false;
+    struct lo_cred old = {};
 
     mapped_name = NULL;
     name = in_name;
@@ -3006,6 +3055,26 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
              ", name=%s value=%s size=%zd)\n", ino, name, value, size);
 
     sprintf(procname, "%i", inode->fd);
+    /*
+     * If we are setting posix access acl and if SGID needs to be
+     * cleared, then switch to caller's gid and drop CAP_FSETID
+     * and that should make sure host kernel clears SGID.
+     *
+     * This probably will not work when we support idmapped mounts.
+     * In that case we will need to find a non-root gid and switch
+     * to it. (Instead of gid in request). Fix it when we support
+     * idmapped mounts.
+     */
+    if (lo->posix_acl && !strcmp(name, "system.posix_acl_access")
+        && (extra_flags & FUSE_SETXATTR_ACL_KILL_SGID)) {
+        ret = lo_drop_cap_change_cred(req, &old, false, "FSETID",
+                                      &cap_fsetid_dropped);
+        if (ret) {
+            saverr = ret;
+            goto out;
+        }
+        switched_creds = true;
+    }
     if (S_ISREG(inode->filetype) || S_ISDIR(inode->filetype)) {
         fd = openat(lo->proc_self_fd, procname, O_RDONLY);
         if (fd < 0) {
@@ -3021,6 +3090,12 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
         saverr = ret == -1 ? errno : 0;
         FCHDIR_NOFAIL(lo->root.fd);
     }
+    if (switched_creds) {
+        if (cap_fsetid_dropped)
+            lo_restore_cred_gain_cap(&old, false, "FSETID");
+        else
+            lo_restore_cred(&old, false);
+    }
 
 out:
     if (fd >= 0) {
-- 
2.25.4

[Virtio-fs] [PATCH v7 6/7] virtiofsd: Switch creds, drop FSETID for system.posix_acl_access xattr

[Vivek Goyal]

When posix access acls are set on a file, it can lead to adjusting file
permissions (mode) as well. If caller does not have CAP_FSETID and it
also does not have membership of owner group, this will lead to clearing
SGID bit in mode.

Current fuse code is written in such a way that it expects file server
to take care of chaning file mode (permission), if there is a need.
Right now, host kernel does not clear SGID bit because virtiofsd is
running as root and has CAP_FSETID. For host kernel to clear SGID,
virtiofsd need to switch to gid of caller in guest and also drop
CAP_FSETID (if caller did not have it to begin with).

If SGID needs to be cleared, client will set the flag
FUSE_SETXATTR_ACL_KILL_SGID in setxattr request. In that case server
should kill sgid.

Currently just switch to uid/gid of the caller and drop CAP_FSETID
and that should do it.

This should fix the xfstest generic/375 test case.

We don't have to switch uid for this to work. That could be one optimization
that pass a parameter to lo_change_cred() to only switch gid and not uid.

Also this will not work whenever (if ever) we support idmapped mounts. In
that case it is possible that uid/gid in request are 0/0 but still we
need to clear SGID. So we will have to pick a non-root sgid and switch
to that instead. That's an TODO item for future when idmapped mount
support is introduced.

This patch only adds the capability to switch creds and drop FSETID
when acl xattr is set. This does not take affect yet. It can take
affect when next patch adds the capability to enable posix_acl.

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

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 0c9084ea15..113c725def 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -175,6 +175,7 @@ struct lo_data {
     int user_killpriv_v2, killpriv_v2;
     /* If set, virtiofsd is responsible for setting umask during creation */
     bool change_umask;
+    int posix_acl;
 };
 
 static const struct fuse_opt lo_opts[] = {
@@ -1185,6 +1186,51 @@ static void lo_restore_cred(struct lo_cred *old, bool restore_umask)
         umask(old->umask);
 }
 
+/*
+ * A helper to change cred and drop capability. Returns 0 on success and
+ * errno on error
+ */
+static int lo_drop_cap_change_cred(fuse_req_t req, struct lo_cred *old,
+                                   bool change_umask, const char *cap_name,
+                                   bool *cap_dropped)
+{
+    int ret;
+    bool __cap_dropped;
+
+    assert(cap_name);
+
+    ret = drop_effective_cap(cap_name, &__cap_dropped);
+    if (ret) {
+        return ret;
+    }
+
+    ret = lo_change_cred(req, old, change_umask);
+    if (ret) {
+        if (__cap_dropped) {
+            if (gain_effective_cap(cap_name)) {
+                fuse_log(FUSE_LOG_ERR, "Failed to gain CAP_%s\n", cap_name);
+            }
+        }
+    }
+
+    if (cap_dropped) {
+        *cap_dropped = __cap_dropped;
+    }
+    return ret;
+}
+
+static void lo_restore_cred_gain_cap(struct lo_cred *old, bool restore_umask,
+                                     const char *cap_name)
+{
+    assert(cap_name);
+
+    lo_restore_cred(old, restore_umask);
+
+    if (gain_effective_cap(cap_name)) {
+        fuse_log(FUSE_LOG_ERR, "Failed to gain CAP_%s\n", cap_name);
+    }
+}
+
 static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent,
                              const char *name, mode_t mode, dev_t rdev,
                              const char *link)
@@ -2976,6 +3022,9 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
     ssize_t ret;
     int saverr;
     int fd = -1;
+    bool switched_creds = false;
+    bool cap_fsetid_dropped = false;
+    struct lo_cred old = {};
 
     mapped_name = NULL;
     name = in_name;
@@ -3006,6 +3055,26 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
              ", name=%s value=%s size=%zd)\n", ino, name, value, size);
 
     sprintf(procname, "%i", inode->fd);
+    /*
+     * If we are setting posix access acl and if SGID needs to be
+     * cleared, then switch to caller's gid and drop CAP_FSETID
+     * and that should make sure host kernel clears SGID.
+     *
+     * This probably will not work when we support idmapped mounts.
+     * In that case we will need to find a non-root gid and switch
+     * to it. (Instead of gid in request). Fix it when we support
+     * idmapped mounts.
+     */
+    if (lo->posix_acl && !strcmp(name, "system.posix_acl_access")
+        && (extra_flags & FUSE_SETXATTR_ACL_KILL_SGID)) {
+        ret = lo_drop_cap_change_cred(req, &old, false, "FSETID",
+                                      &cap_fsetid_dropped);
+        if (ret) {
+            saverr = ret;
+            goto out;
+        }
+        switched_creds = true;
+    }
     if (S_ISREG(inode->filetype) || S_ISDIR(inode->filetype)) {
         fd = openat(lo->proc_self_fd, procname, O_RDONLY);
         if (fd < 0) {
@@ -3021,6 +3090,12 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
         saverr = ret == -1 ? errno : 0;
         FCHDIR_NOFAIL(lo->root.fd);
     }
+    if (switched_creds) {
+        if (cap_fsetid_dropped)
+            lo_restore_cred_gain_cap(&old, false, "FSETID");
+        else
+            lo_restore_cred(&old, false);
+    }
 
 out:
     if (fd >= 0) {
-- 
2.25.4

[PATCH v7 7/7] 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. 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 | 115 ++++++++++++++++++++++++++++++-
 3 files changed, 118 insertions(+), 1 deletion(-)

diff --git a/docs/tools/virtiofsd.rst b/docs/tools/virtiofsd.rst
index 00554c75bd..a41f934999 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 5e98ed702b..a8295d975a 100644
--- a/tools/virtiofsd/helper.c
+++ b/tools/virtiofsd/helper.c
@@ -186,6 +186,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 113c725def..e80fd76d71 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -175,7 +175,7 @@ struct lo_data {
     int user_killpriv_v2, killpriv_v2;
     /* If set, virtiofsd is responsible for setting umask during creation */
     bool change_umask;
-    int posix_acl;
+    int user_posix_acl, posix_acl;
 };
 
 static const struct fuse_opt lo_opts[] = {
@@ -208,6 +208,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;
@@ -706,6 +708,32 @@ 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, print error message
+         * now. It will fail later in fuse_lowlevel.c
+         */
+        if (!(conn->capable & FUSE_CAP_POSIX_ACL) ||
+            !(conn->capable & FUSE_CAP_DONT_MASK) ||
+            !(conn->capable & FUSE_CAP_SETXATTR_EXT)) {
+            fuse_log(FUSE_LOG_ERR, "lo_init: Can not enable posix acl."
+                     " kernel does not support FUSE_POSIX_ACL, FUSE_DONT_MASK"
+                     " or FUSE_SETXATTR_EXT capability.\n");
+        } else {
+            fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling posix acl\n");
+        }
+
+        conn->want |= FUSE_CAP_POSIX_ACL | FUSE_CAP_DONT_MASK |
+                      FUSE_CAP_SETXATTR_EXT;
+        lo->change_umask = true;
+        lo->posix_acl = 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,
@@ -2783,6 +2811,63 @@ static int xattr_map_server(const struct lo_data *lo, const char *server_name,
         assert(fchdir_res == 0);                       \
     } while (0)
 
+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)
 {
@@ -2796,6 +2881,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) {
@@ -2986,6 +3076,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 {
         /*
@@ -3026,6 +3122,11 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
     bool cap_fsetid_dropped = false;
     struct lo_cred old = {};
 
+    if (block_xattr(lo, in_name)) {
+        fuse_reply_err(req, EOPNOTSUPP);
+        return;
+    }
+
     mapped_name = NULL;
     name = in_name;
     if (lo->xattrmap) {
@@ -3118,6 +3219,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) {
@@ -3812,6 +3918,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;
@@ -3940,6 +4047,12 @@ 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

[Virtio-fs] [PATCH v7 7/7] 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. 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 | 115 ++++++++++++++++++++++++++++++-
 3 files changed, 118 insertions(+), 1 deletion(-)

diff --git a/docs/tools/virtiofsd.rst b/docs/tools/virtiofsd.rst
index 00554c75bd..a41f934999 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 5e98ed702b..a8295d975a 100644
--- a/tools/virtiofsd/helper.c
+++ b/tools/virtiofsd/helper.c
@@ -186,6 +186,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 113c725def..e80fd76d71 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -175,7 +175,7 @@ struct lo_data {
     int user_killpriv_v2, killpriv_v2;
     /* If set, virtiofsd is responsible for setting umask during creation */
     bool change_umask;
-    int posix_acl;
+    int user_posix_acl, posix_acl;
 };
 
 static const struct fuse_opt lo_opts[] = {
@@ -208,6 +208,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;
@@ -706,6 +708,32 @@ 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, print error message
+         * now. It will fail later in fuse_lowlevel.c
+         */
+        if (!(conn->capable & FUSE_CAP_POSIX_ACL) ||
+            !(conn->capable & FUSE_CAP_DONT_MASK) ||
+            !(conn->capable & FUSE_CAP_SETXATTR_EXT)) {
+            fuse_log(FUSE_LOG_ERR, "lo_init: Can not enable posix acl."
+                     " kernel does not support FUSE_POSIX_ACL, FUSE_DONT_MASK"
+                     " or FUSE_SETXATTR_EXT capability.\n");
+        } else {
+            fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling posix acl\n");
+        }
+
+        conn->want |= FUSE_CAP_POSIX_ACL | FUSE_CAP_DONT_MASK |
+                      FUSE_CAP_SETXATTR_EXT;
+        lo->change_umask = true;
+        lo->posix_acl = 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,
@@ -2783,6 +2811,63 @@ static int xattr_map_server(const struct lo_data *lo, const char *server_name,
         assert(fchdir_res == 0);                       \
     } while (0)
 
+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)
 {
@@ -2796,6 +2881,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) {
@@ -2986,6 +3076,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 {
         /*
@@ -3026,6 +3122,11 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
     bool cap_fsetid_dropped = false;
     struct lo_cred old = {};
 
+    if (block_xattr(lo, in_name)) {
+        fuse_reply_err(req, EOPNOTSUPP);
+        return;
+    }
+
     mapped_name = NULL;
     name = in_name;
     if (lo->xattrmap) {
@@ -3118,6 +3219,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) {
@@ -3812,6 +3918,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;
@@ -3940,6 +4047,12 @@ 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

Re: [PATCH v7 1/7] virtiofsd: Fix fuse setxattr() API change issue

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> With kernel header updates fuse_setxattr_in struct has grown in size.
> But this new struct size only takes affect if user has opted in
> for fuse feature FUSE_SETXATTR_EXT otherwise fuse continues to
> send "fuse_setxattr_in" of older size. Older size is determined
> by FUSE_COMPAT_SETXATTR_IN_SIZE.
> 
> Fix this. If we have not opted in for FUSE_SETXATTR_EXT, then
> expect that we will get fuse_setxattr_in of size FUSE_COMPAT_SETXATTR_IN_SIZE
> and not sizeof(struct fuse_sexattr_in).
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Yeh it's a bit of a grim fix, but I think it's the best we can do, and
we need to get it in since the headers have already been merged.
(I don't think libfuse has a fix for this in yet itself, but it might
survive because it doesn't bother copying the data like we do with
mbuf).


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  tools/virtiofsd/fuse_common.h   | 5 +++++
>  tools/virtiofsd/fuse_lowlevel.c | 7 ++++++-
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/virtiofsd/fuse_common.h b/tools/virtiofsd/fuse_common.h
> index fa9671872e..0c2665b977 100644
> --- a/tools/virtiofsd/fuse_common.h
> +++ b/tools/virtiofsd/fuse_common.h
> @@ -372,6 +372,11 @@ struct fuse_file_info {
>   */
>  #define FUSE_CAP_HANDLE_KILLPRIV_V2 (1 << 28)
>  
> +/**
> + * Indicates that file server supports extended struct fuse_setxattr_in
> + */
> +#define FUSE_CAP_SETXATTR_EXT (1 << 29)
> +
>  /**
>   * Ioctl flags
>   *
> diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
> index 7fe2cef1eb..c2b6ff1686 100644
> --- a/tools/virtiofsd/fuse_lowlevel.c
> +++ b/tools/virtiofsd/fuse_lowlevel.c
> @@ -1419,8 +1419,13 @@ static void do_setxattr(fuse_req_t req, fuse_ino_t nodeid,
>      struct fuse_setxattr_in *arg;
>      const char *name;
>      const char *value;
> +    bool setxattr_ext = req->se->conn.want & FUSE_CAP_SETXATTR_EXT;
>  
> -    arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
> +    if (setxattr_ext) {
> +        arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
> +    } else {
> +        arg = fuse_mbuf_iter_advance(iter, FUSE_COMPAT_SETXATTR_IN_SIZE);
> +    }
>      name = fuse_mbuf_iter_advance_str(iter);
>      if (!arg || !name) {
>          fuse_reply_err(req, EINVAL);
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH v7 1/7] virtiofsd: Fix fuse setxattr() API change issue

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> With kernel header updates fuse_setxattr_in struct has grown in size.
> But this new struct size only takes affect if user has opted in
> for fuse feature FUSE_SETXATTR_EXT otherwise fuse continues to
> send "fuse_setxattr_in" of older size. Older size is determined
> by FUSE_COMPAT_SETXATTR_IN_SIZE.
> 
> Fix this. If we have not opted in for FUSE_SETXATTR_EXT, then
> expect that we will get fuse_setxattr_in of size FUSE_COMPAT_SETXATTR_IN_SIZE
> and not sizeof(struct fuse_sexattr_in).
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Yeh it's a bit of a grim fix, but I think it's the best we can do, and
we need to get it in since the headers have already been merged.
(I don't think libfuse has a fix for this in yet itself, but it might
survive because it doesn't bother copying the data like we do with
mbuf).


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  tools/virtiofsd/fuse_common.h   | 5 +++++
>  tools/virtiofsd/fuse_lowlevel.c | 7 ++++++-
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/virtiofsd/fuse_common.h b/tools/virtiofsd/fuse_common.h
> index fa9671872e..0c2665b977 100644
> --- a/tools/virtiofsd/fuse_common.h
> +++ b/tools/virtiofsd/fuse_common.h
> @@ -372,6 +372,11 @@ struct fuse_file_info {
>   */
>  #define FUSE_CAP_HANDLE_KILLPRIV_V2 (1 << 28)
>  
> +/**
> + * Indicates that file server supports extended struct fuse_setxattr_in
> + */
> +#define FUSE_CAP_SETXATTR_EXT (1 << 29)
> +
>  /**
>   * Ioctl flags
>   *
> diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
> index 7fe2cef1eb..c2b6ff1686 100644
> --- a/tools/virtiofsd/fuse_lowlevel.c
> +++ b/tools/virtiofsd/fuse_lowlevel.c
> @@ -1419,8 +1419,13 @@ static void do_setxattr(fuse_req_t req, fuse_ino_t nodeid,
>      struct fuse_setxattr_in *arg;
>      const char *name;
>      const char *value;
> +    bool setxattr_ext = req->se->conn.want & FUSE_CAP_SETXATTR_EXT;
>  
> -    arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
> +    if (setxattr_ext) {
> +        arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
> +    } else {
> +        arg = fuse_mbuf_iter_advance(iter, FUSE_COMPAT_SETXATTR_IN_SIZE);
> +    }
>      name = fuse_mbuf_iter_advance_str(iter);
>      if (!arg || !name) {
>          fuse_reply_err(req, EINVAL);
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH v7 1/7] virtiofsd: Fix fuse setxattr() API change issue

[Vivek Goyal]

On Mon, Jun 28, 2021 at 03:46:40PM +0100, Dr. David Alan Gilbert wrote:
> * Vivek Goyal (vgoyal@redhat.com) wrote:
> > With kernel header updates fuse_setxattr_in struct has grown in size.
> > But this new struct size only takes affect if user has opted in
> > for fuse feature FUSE_SETXATTR_EXT otherwise fuse continues to
> > send "fuse_setxattr_in" of older size. Older size is determined
> > by FUSE_COMPAT_SETXATTR_IN_SIZE.
> > 
> > Fix this. If we have not opted in for FUSE_SETXATTR_EXT, then
> > expect that we will get fuse_setxattr_in of size FUSE_COMPAT_SETXATTR_IN_SIZE
> > and not sizeof(struct fuse_sexattr_in).
> > 
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> 
> Yeh it's a bit of a grim fix, but I think it's the best we can do, and
> we need to get it in since the headers have already been merged.
> (I don't think libfuse has a fix for this in yet itself, but it might
> survive because it doesn't bother copying the data like we do with
> mbuf).

[ CC Niklaus Rath]

libfuse will need a fix as well once they move to 5.13 kernel headers.
As of now they seem to be still working with 5.2 kernel headers.

commit 249942f6411042c4af18bd10438da34801cce02b
Author: Kirill Smelkov <kirr@nexedi.com>
Date:   Tue Jul 9 23:54:09 2019 +0300

    Sync fuse_kernel.h with Linux 5.2 (#433)

Thanks
Vivek

> 
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> > ---
> >  tools/virtiofsd/fuse_common.h   | 5 +++++
> >  tools/virtiofsd/fuse_lowlevel.c | 7 ++++++-
> >  2 files changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/virtiofsd/fuse_common.h b/tools/virtiofsd/fuse_common.h
> > index fa9671872e..0c2665b977 100644
> > --- a/tools/virtiofsd/fuse_common.h
> > +++ b/tools/virtiofsd/fuse_common.h
> > @@ -372,6 +372,11 @@ struct fuse_file_info {
> >   */
> >  #define FUSE_CAP_HANDLE_KILLPRIV_V2 (1 << 28)
> >  
> > +/**
> > + * Indicates that file server supports extended struct fuse_setxattr_in
> > + */
> > +#define FUSE_CAP_SETXATTR_EXT (1 << 29)
> > +
> >  /**
> >   * Ioctl flags
> >   *
> > diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
> > index 7fe2cef1eb..c2b6ff1686 100644
> > --- a/tools/virtiofsd/fuse_lowlevel.c
> > +++ b/tools/virtiofsd/fuse_lowlevel.c
> > @@ -1419,8 +1419,13 @@ static void do_setxattr(fuse_req_t req, fuse_ino_t nodeid,
> >      struct fuse_setxattr_in *arg;
> >      const char *name;
> >      const char *value;
> > +    bool setxattr_ext = req->se->conn.want & FUSE_CAP_SETXATTR_EXT;
> >  
> > -    arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
> > +    if (setxattr_ext) {
> > +        arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
> > +    } else {
> > +        arg = fuse_mbuf_iter_advance(iter, FUSE_COMPAT_SETXATTR_IN_SIZE);
> > +    }
> >      name = fuse_mbuf_iter_advance_str(iter);
> >      if (!arg || !name) {
> >          fuse_reply_err(req, EINVAL);
> > -- 
> > 2.25.4
> > 
> -- 
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://listman.redhat.com/mailman/listinfo/virtio-fs
> 

Re: [PATCH v7 2/7] virtiofsd: Fix xattr operations overwriting errno

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> getxattr/setxattr/removexattr/listxattr operations handle regualar
> and non-regular files differently. For the case of non-regular files
> we do fchdir(/proc/self/fd) and the xattr operation and then revert
> back to original working directory. After this we are saving errno
> and that's buggy because fchdir() will overwrite the errno.
> 
> FCHDIR_NOFAIL(lo->proc_self_fd);
> ret = getxattr(procname, name, value, size);
> FCHDIR_NOFAIL(lo->root.fd);
> 
> if (ret == -1)
>     saverr = errno
> 
> In above example, if getxattr() failed, we will still return 0 to caller
> as errno must have been written by FCHDIR_NOFAIL(lo->root.fd) call.
> Fix all such instances and capture "errno" early and save in "saverr"
> variable.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

I think the existing code is actually safe; I don't think fchdir
will/should set errno unless there's an error, and we're explictly
asserting there isn't one.

However, I do prefer doing this save since we already have the save
variables and it makes the chance of screwing it up from any other
forgotten syscall less likely, so


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  tools/virtiofsd/passthrough_ll.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 49c21fd855..ec91b3c133 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -2791,15 +2791,17 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
>              goto out_err;
>          }
>          ret = fgetxattr(fd, name, value, size);
> +        saverr = ret == -1 ? errno : 0;
>      } else {
>          /* fchdir should not fail here */
>          FCHDIR_NOFAIL(lo->proc_self_fd);
>          ret = getxattr(procname, name, value, size);
> +        saverr = ret == -1 ? errno : 0;
>          FCHDIR_NOFAIL(lo->root.fd);
>      }
>  
>      if (ret == -1) {
> -        goto out_err;
> +        goto out;
>      }
>      if (size) {
>          saverr = 0;
> @@ -2864,15 +2866,17 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
>              goto out_err;
>          }
>          ret = flistxattr(fd, value, size);
> +        saverr = ret == -1 ? errno : 0;
>      } else {
>          /* fchdir should not fail here */
>          FCHDIR_NOFAIL(lo->proc_self_fd);
>          ret = listxattr(procname, value, size);
> +        saverr = ret == -1 ? errno : 0;
>          FCHDIR_NOFAIL(lo->root.fd);
>      }
>  
>      if (ret == -1) {
> -        goto out_err;
> +        goto out;
>      }
>      if (size) {
>          saverr = 0;
> @@ -2998,15 +3002,15 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
>              goto out;
>          }
>          ret = fsetxattr(fd, name, value, size, flags);
> +        saverr = ret == -1 ? errno : 0;
>      } else {
>          /* fchdir should not fail here */
>          FCHDIR_NOFAIL(lo->proc_self_fd);
>          ret = setxattr(procname, name, value, size, flags);
> +        saverr = ret == -1 ? errno : 0;
>          FCHDIR_NOFAIL(lo->root.fd);
>      }
>  
> -    saverr = ret == -1 ? errno : 0;
> -
>  out:
>      if (fd >= 0) {
>          close(fd);
> @@ -3064,15 +3068,15 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *in_name)
>              goto out;
>          }
>          ret = fremovexattr(fd, name);
> +        saverr = ret == -1 ? errno : 0;
>      } else {
>          /* fchdir should not fail here */
>          FCHDIR_NOFAIL(lo->proc_self_fd);
>          ret = removexattr(procname, name);
> +        saverr = ret == -1 ? errno : 0;
>          FCHDIR_NOFAIL(lo->root.fd);
>      }
>  
> -    saverr = ret == -1 ? errno : 0;
> -
>  out:
>      if (fd >= 0) {
>          close(fd);
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH v7 2/7] virtiofsd: Fix xattr operations overwriting errno

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> getxattr/setxattr/removexattr/listxattr operations handle regualar
> and non-regular files differently. For the case of non-regular files
> we do fchdir(/proc/self/fd) and the xattr operation and then revert
> back to original working directory. After this we are saving errno
> and that's buggy because fchdir() will overwrite the errno.
> 
> FCHDIR_NOFAIL(lo->proc_self_fd);
> ret = getxattr(procname, name, value, size);
> FCHDIR_NOFAIL(lo->root.fd);
> 
> if (ret == -1)
>     saverr = errno
> 
> In above example, if getxattr() failed, we will still return 0 to caller
> as errno must have been written by FCHDIR_NOFAIL(lo->root.fd) call.
> Fix all such instances and capture "errno" early and save in "saverr"
> variable.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

I think the existing code is actually safe; I don't think fchdir
will/should set errno unless there's an error, and we're explictly
asserting there isn't one.

However, I do prefer doing this save since we already have the save
variables and it makes the chance of screwing it up from any other
forgotten syscall less likely, so


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  tools/virtiofsd/passthrough_ll.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 49c21fd855..ec91b3c133 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -2791,15 +2791,17 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
>              goto out_err;
>          }
>          ret = fgetxattr(fd, name, value, size);
> +        saverr = ret == -1 ? errno : 0;
>      } else {
>          /* fchdir should not fail here */
>          FCHDIR_NOFAIL(lo->proc_self_fd);
>          ret = getxattr(procname, name, value, size);
> +        saverr = ret == -1 ? errno : 0;
>          FCHDIR_NOFAIL(lo->root.fd);
>      }
>  
>      if (ret == -1) {
> -        goto out_err;
> +        goto out;
>      }
>      if (size) {
>          saverr = 0;
> @@ -2864,15 +2866,17 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
>              goto out_err;
>          }
>          ret = flistxattr(fd, value, size);
> +        saverr = ret == -1 ? errno : 0;
>      } else {
>          /* fchdir should not fail here */
>          FCHDIR_NOFAIL(lo->proc_self_fd);
>          ret = listxattr(procname, value, size);
> +        saverr = ret == -1 ? errno : 0;
>          FCHDIR_NOFAIL(lo->root.fd);
>      }
>  
>      if (ret == -1) {
> -        goto out_err;
> +        goto out;
>      }
>      if (size) {
>          saverr = 0;
> @@ -2998,15 +3002,15 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
>              goto out;
>          }
>          ret = fsetxattr(fd, name, value, size, flags);
> +        saverr = ret == -1 ? errno : 0;
>      } else {
>          /* fchdir should not fail here */
>          FCHDIR_NOFAIL(lo->proc_self_fd);
>          ret = setxattr(procname, name, value, size, flags);
> +        saverr = ret == -1 ? errno : 0;
>          FCHDIR_NOFAIL(lo->root.fd);
>      }
>  
> -    saverr = ret == -1 ? errno : 0;
> -
>  out:
>      if (fd >= 0) {
>          close(fd);
> @@ -3064,15 +3068,15 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *in_name)
>              goto out;
>          }
>          ret = fremovexattr(fd, name);
> +        saverr = ret == -1 ? errno : 0;
>      } else {
>          /* fchdir should not fail here */
>          FCHDIR_NOFAIL(lo->proc_self_fd);
>          ret = removexattr(procname, name);
> +        saverr = ret == -1 ? errno : 0;
>          FCHDIR_NOFAIL(lo->root.fd);
>      }
>  
> -    saverr = ret == -1 ? errno : 0;
> -
>  out:
>      if (fd >= 0) {
>          close(fd);
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v7 3/7] virtiofsd: Add support for extended setxattr

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> Add the bits to enable support for setxattr_ext if fuse offers it. Do not
> enable it by default yet. Let passthrough_ll opt-in. Enabling it by deafult
> kind of automatically means that you are taking responsibility of clearing
> SGID if ACL is set.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  tools/virtiofsd/fuse_common.h    |  5 +++++
>  tools/virtiofsd/fuse_lowlevel.c  | 11 ++++++++++-
>  tools/virtiofsd/fuse_lowlevel.h  |  3 ++-
>  tools/virtiofsd/passthrough_ll.c |  3 ++-
>  4 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/virtiofsd/fuse_common.h b/tools/virtiofsd/fuse_common.h
> index 0c2665b977..8abac80098 100644
> --- a/tools/virtiofsd/fuse_common.h
> +++ b/tools/virtiofsd/fuse_common.h
> @@ -377,6 +377,11 @@ struct fuse_file_info {
>   */
>  #define FUSE_CAP_SETXATTR_EXT (1 << 29)
>  
> +/**
> + * Indicates that file server supports extended struct fuse_setxattr_in
> + */
> +#define FUSE_CAP_SETXATTR_EXT (1 << 29)
> +

You already added that in 1/7 - but other than that, 


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

>  /**
>   * Ioctl flags
>   *
> diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
> index c2b6ff1686..d1e24c013f 100644
> --- a/tools/virtiofsd/fuse_lowlevel.c
> +++ b/tools/virtiofsd/fuse_lowlevel.c
> @@ -1439,7 +1439,9 @@ static void do_setxattr(fuse_req_t req, fuse_ino_t nodeid,
>      }
>  
>      if (req->se->op.setxattr) {
> -        req->se->op.setxattr(req, nodeid, name, value, arg->size, arg->flags);
> +        uint32_t setxattr_flags = setxattr_ext ? arg->setxattr_flags : 0;
> +        req->se->op.setxattr(req, nodeid, name, value, arg->size, arg->flags,
> +                             setxattr_flags);
>      } else {
>          fuse_reply_err(req, ENOSYS);
>      }
> @@ -1986,6 +1988,9 @@ static void do_init(fuse_req_t req, fuse_ino_t nodeid,
>      if (arg->flags & FUSE_HANDLE_KILLPRIV_V2) {
>          se->conn.capable |= FUSE_CAP_HANDLE_KILLPRIV_V2;
>      }
> +    if (arg->flags & FUSE_SETXATTR_EXT) {
> +        se->conn.capable |= FUSE_CAP_SETXATTR_EXT;
> +    }
>  #ifdef HAVE_SPLICE
>  #ifdef HAVE_VMSPLICE
>      se->conn.capable |= FUSE_CAP_SPLICE_WRITE | FUSE_CAP_SPLICE_MOVE;
> @@ -2121,6 +2126,10 @@ static void do_init(fuse_req_t req, fuse_ino_t nodeid,
>          outarg.flags |= FUSE_HANDLE_KILLPRIV_V2;
>      }
>  
> +    if (se->conn.want & FUSE_CAP_SETXATTR_EXT) {
> +        outarg.flags |= FUSE_SETXATTR_EXT;
> +    }
> +
>      fuse_log(FUSE_LOG_DEBUG, "   INIT: %u.%u\n", outarg.major, outarg.minor);
>      fuse_log(FUSE_LOG_DEBUG, "   flags=0x%08x\n", outarg.flags);
>      fuse_log(FUSE_LOG_DEBUG, "   max_readahead=0x%08x\n", outarg.max_readahead);
> diff --git a/tools/virtiofsd/fuse_lowlevel.h b/tools/virtiofsd/fuse_lowlevel.h
> index 3bf786b034..4b4e8c9724 100644
> --- a/tools/virtiofsd/fuse_lowlevel.h
> +++ b/tools/virtiofsd/fuse_lowlevel.h
> @@ -798,7 +798,8 @@ struct fuse_lowlevel_ops {
>       *   fuse_reply_err
>       */
>      void (*setxattr)(fuse_req_t req, fuse_ino_t ino, const char *name,
> -                     const char *value, size_t size, int flags);
> +                     const char *value, size_t size, int flags,
> +                     uint32_t setxattr_flags);
>  
>      /**
>       * Get an extended attribute
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index ec91b3c133..9f5cd98fb5 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -2955,7 +2955,8 @@ out:
>  }
>  
>  static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
> -                        const char *value, size_t size, int flags)
> +                        const char *value, size_t size, int flags,
> +                        uint32_t extra_flags)
>  {
>      char procname[64];
>      const char *name;
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH v7 3/7] virtiofsd: Add support for extended setxattr

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> Add the bits to enable support for setxattr_ext if fuse offers it. Do not
> enable it by default yet. Let passthrough_ll opt-in. Enabling it by deafult
> kind of automatically means that you are taking responsibility of clearing
> SGID if ACL is set.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  tools/virtiofsd/fuse_common.h    |  5 +++++
>  tools/virtiofsd/fuse_lowlevel.c  | 11 ++++++++++-
>  tools/virtiofsd/fuse_lowlevel.h  |  3 ++-
>  tools/virtiofsd/passthrough_ll.c |  3 ++-
>  4 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/virtiofsd/fuse_common.h b/tools/virtiofsd/fuse_common.h
> index 0c2665b977..8abac80098 100644
> --- a/tools/virtiofsd/fuse_common.h
> +++ b/tools/virtiofsd/fuse_common.h
> @@ -377,6 +377,11 @@ struct fuse_file_info {
>   */
>  #define FUSE_CAP_SETXATTR_EXT (1 << 29)
>  
> +/**
> + * Indicates that file server supports extended struct fuse_setxattr_in
> + */
> +#define FUSE_CAP_SETXATTR_EXT (1 << 29)
> +

You already added that in 1/7 - but other than that, 


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

>  /**
>   * Ioctl flags
>   *
> diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
> index c2b6ff1686..d1e24c013f 100644
> --- a/tools/virtiofsd/fuse_lowlevel.c
> +++ b/tools/virtiofsd/fuse_lowlevel.c
> @@ -1439,7 +1439,9 @@ static void do_setxattr(fuse_req_t req, fuse_ino_t nodeid,
>      }
>  
>      if (req->se->op.setxattr) {
> -        req->se->op.setxattr(req, nodeid, name, value, arg->size, arg->flags);
> +        uint32_t setxattr_flags = setxattr_ext ? arg->setxattr_flags : 0;
> +        req->se->op.setxattr(req, nodeid, name, value, arg->size, arg->flags,
> +                             setxattr_flags);
>      } else {
>          fuse_reply_err(req, ENOSYS);
>      }
> @@ -1986,6 +1988,9 @@ static void do_init(fuse_req_t req, fuse_ino_t nodeid,
>      if (arg->flags & FUSE_HANDLE_KILLPRIV_V2) {
>          se->conn.capable |= FUSE_CAP_HANDLE_KILLPRIV_V2;
>      }
> +    if (arg->flags & FUSE_SETXATTR_EXT) {
> +        se->conn.capable |= FUSE_CAP_SETXATTR_EXT;
> +    }
>  #ifdef HAVE_SPLICE
>  #ifdef HAVE_VMSPLICE
>      se->conn.capable |= FUSE_CAP_SPLICE_WRITE | FUSE_CAP_SPLICE_MOVE;
> @@ -2121,6 +2126,10 @@ static void do_init(fuse_req_t req, fuse_ino_t nodeid,
>          outarg.flags |= FUSE_HANDLE_KILLPRIV_V2;
>      }
>  
> +    if (se->conn.want & FUSE_CAP_SETXATTR_EXT) {
> +        outarg.flags |= FUSE_SETXATTR_EXT;
> +    }
> +
>      fuse_log(FUSE_LOG_DEBUG, "   INIT: %u.%u\n", outarg.major, outarg.minor);
>      fuse_log(FUSE_LOG_DEBUG, "   flags=0x%08x\n", outarg.flags);
>      fuse_log(FUSE_LOG_DEBUG, "   max_readahead=0x%08x\n", outarg.max_readahead);
> diff --git a/tools/virtiofsd/fuse_lowlevel.h b/tools/virtiofsd/fuse_lowlevel.h
> index 3bf786b034..4b4e8c9724 100644
> --- a/tools/virtiofsd/fuse_lowlevel.h
> +++ b/tools/virtiofsd/fuse_lowlevel.h
> @@ -798,7 +798,8 @@ struct fuse_lowlevel_ops {
>       *   fuse_reply_err
>       */
>      void (*setxattr)(fuse_req_t req, fuse_ino_t ino, const char *name,
> -                     const char *value, size_t size, int flags);
> +                     const char *value, size_t size, int flags,
> +                     uint32_t setxattr_flags);
>  
>      /**
>       * Get an extended attribute
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index ec91b3c133..9f5cd98fb5 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -2955,7 +2955,8 @@ out:
>  }
>  
>  static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
> -                        const char *value, size_t size, int flags)
> +                        const char *value, size_t size, int flags,
> +                        uint32_t extra_flags)
>  {
>      char procname[64];
>      const char *name;
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v7 5/7] virtiofsd: Add capability to change/restore umask

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> 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 few patches 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 9f5cd98fb5..0c9084ea15 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -122,6 +122,7 @@ struct lo_inode {
>  struct lo_cred {
>      uid_t euid;
>      gid_t egid;
> +    mode_t umask;
>  };
>  
>  enum {
> @@ -172,6 +173,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[] = {
> @@ -1134,7 +1137,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;
>  
> @@ -1154,11 +1158,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;
>  
> @@ -1173,6 +1180,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,
> @@ -1202,7 +1212,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));

Can you explain what these ISLNK checks are for (insid mknod_symlink, so
is that always true or irrelevant?)

Dave

>      if (saverr) {
>          goto out;
>      }
> @@ -1211,7 +1221,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;
> @@ -1917,7 +1927,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;
>      }
> @@ -1928,7 +1938,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
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH v7 5/7] virtiofsd: Add capability to change/restore umask

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> 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 few patches 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 9f5cd98fb5..0c9084ea15 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -122,6 +122,7 @@ struct lo_inode {
>  struct lo_cred {
>      uid_t euid;
>      gid_t egid;
> +    mode_t umask;
>  };
>  
>  enum {
> @@ -172,6 +173,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[] = {
> @@ -1134,7 +1137,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;
>  
> @@ -1154,11 +1158,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;
>  
> @@ -1173,6 +1180,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,
> @@ -1202,7 +1212,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));

Can you explain what these ISLNK checks are for (insid mknod_symlink, so
is that always true or irrelevant?)

Dave

>      if (saverr) {
>          goto out;
>      }
> @@ -1211,7 +1221,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;
> @@ -1917,7 +1927,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;
>      }
> @@ -1928,7 +1938,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
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v7 6/7] virtiofsd: Switch creds, drop FSETID for system.posix_acl_access xattr

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> When posix access acls are set on a file, it can lead to adjusting file
> permissions (mode) as well. If caller does not have CAP_FSETID and it
> also does not have membership of owner group, this will lead to clearing
> SGID bit in mode.
> 
> Current fuse code is written in such a way that it expects file server
> to take care of chaning file mode (permission), if there is a need.
> Right now, host kernel does not clear SGID bit because virtiofsd is
> running as root and has CAP_FSETID. For host kernel to clear SGID,
> virtiofsd need to switch to gid of caller in guest and also drop
> CAP_FSETID (if caller did not have it to begin with).
> 
> If SGID needs to be cleared, client will set the flag
> FUSE_SETXATTR_ACL_KILL_SGID in setxattr request. In that case server
> should kill sgid.
> 
> Currently just switch to uid/gid of the caller and drop CAP_FSETID
> and that should do it.
> 
> This should fix the xfstest generic/375 test case.
> 
> We don't have to switch uid for this to work. That could be one optimization
> that pass a parameter to lo_change_cred() to only switch gid and not uid.
> 
> Also this will not work whenever (if ever) we support idmapped mounts. In
> that case it is possible that uid/gid in request are 0/0 but still we
> need to clear SGID. So we will have to pick a non-root sgid and switch
> to that instead. That's an TODO item for future when idmapped mount
> support is introduced.
> 
> This patch only adds the capability to switch creds and drop FSETID
> when acl xattr is set. This does not take affect yet. It can take
> affect when next patch adds the capability to enable posix_acl.
> 
> Reported-by: Luis Henriques <lhenriques@suse.de>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  tools/virtiofsd/passthrough_ll.c | 75 ++++++++++++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 0c9084ea15..113c725def 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -175,6 +175,7 @@ struct lo_data {
>      int user_killpriv_v2, killpriv_v2;
>      /* If set, virtiofsd is responsible for setting umask during creation */
>      bool change_umask;
> +    int posix_acl;
>  };
>  
>  static const struct fuse_opt lo_opts[] = {
> @@ -1185,6 +1186,51 @@ static void lo_restore_cred(struct lo_cred *old, bool restore_umask)
>          umask(old->umask);
>  }
>  
> +/*
> + * A helper to change cred and drop capability. Returns 0 on success and
> + * errno on error
> + */
> +static int lo_drop_cap_change_cred(fuse_req_t req, struct lo_cred *old,
> +                                   bool change_umask, const char *cap_name,
> +                                   bool *cap_dropped)
> +{
> +    int ret;
> +    bool __cap_dropped;
> +
> +    assert(cap_name);
> +
> +    ret = drop_effective_cap(cap_name, &__cap_dropped);
> +    if (ret) {
> +        return ret;
> +    }
> +
> +    ret = lo_change_cred(req, old, change_umask);
> +    if (ret) {
> +        if (__cap_dropped) {
> +            if (gain_effective_cap(cap_name)) {
> +                fuse_log(FUSE_LOG_ERR, "Failed to gain CAP_%s\n", cap_name);
> +            }
> +        }
> +    }
> +
> +    if (cap_dropped) {
> +        *cap_dropped = __cap_dropped;
> +    }
> +    return ret;
> +}
> +
> +static void lo_restore_cred_gain_cap(struct lo_cred *old, bool restore_umask,
> +                                     const char *cap_name)
> +{
> +    assert(cap_name);
> +
> +    lo_restore_cred(old, restore_umask);
> +
> +    if (gain_effective_cap(cap_name)) {
> +        fuse_log(FUSE_LOG_ERR, "Failed to gain CAP_%s\n", cap_name);
> +    }
> +}
> +
>  static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent,
>                               const char *name, mode_t mode, dev_t rdev,
>                               const char *link)
> @@ -2976,6 +3022,9 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
>      ssize_t ret;
>      int saverr;
>      int fd = -1;
> +    bool switched_creds = false;
> +    bool cap_fsetid_dropped = false;
> +    struct lo_cred old = {};
>  
>      mapped_name = NULL;
>      name = in_name;
> @@ -3006,6 +3055,26 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
>               ", name=%s value=%s size=%zd)\n", ino, name, value, size);
>  
>      sprintf(procname, "%i", inode->fd);
> +    /*
> +     * If we are setting posix access acl and if SGID needs to be
> +     * cleared, then switch to caller's gid and drop CAP_FSETID
> +     * and that should make sure host kernel clears SGID.
> +     *
> +     * This probably will not work when we support idmapped mounts.
> +     * In that case we will need to find a non-root gid and switch
> +     * to it. (Instead of gid in request). Fix it when we support
> +     * idmapped mounts.
> +     */
> +    if (lo->posix_acl && !strcmp(name, "system.posix_acl_access")
> +        && (extra_flags & FUSE_SETXATTR_ACL_KILL_SGID)) {
> +        ret = lo_drop_cap_change_cred(req, &old, false, "FSETID",
> +                                      &cap_fsetid_dropped);
> +        if (ret) {
> +            saverr = ret;
> +            goto out;
> +        }
> +        switched_creds = true;
> +    }
>      if (S_ISREG(inode->filetype) || S_ISDIR(inode->filetype)) {
>          fd = openat(lo->proc_self_fd, procname, O_RDONLY);
>          if (fd < 0) {
> @@ -3021,6 +3090,12 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
>          saverr = ret == -1 ? errno : 0;
>          FCHDIR_NOFAIL(lo->root.fd);
>      }
> +    if (switched_creds) {
> +        if (cap_fsetid_dropped)
> +            lo_restore_cred_gain_cap(&old, false, "FSETID");
> +        else
> +            lo_restore_cred(&old, false);
> +    }
>  
>  out:
>      if (fd >= 0) {
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH v7 6/7] virtiofsd: Switch creds, drop FSETID for system.posix_acl_access xattr

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> When posix access acls are set on a file, it can lead to adjusting file
> permissions (mode) as well. If caller does not have CAP_FSETID and it
> also does not have membership of owner group, this will lead to clearing
> SGID bit in mode.
> 
> Current fuse code is written in such a way that it expects file server
> to take care of chaning file mode (permission), if there is a need.
> Right now, host kernel does not clear SGID bit because virtiofsd is
> running as root and has CAP_FSETID. For host kernel to clear SGID,
> virtiofsd need to switch to gid of caller in guest and also drop
> CAP_FSETID (if caller did not have it to begin with).
> 
> If SGID needs to be cleared, client will set the flag
> FUSE_SETXATTR_ACL_KILL_SGID in setxattr request. In that case server
> should kill sgid.
> 
> Currently just switch to uid/gid of the caller and drop CAP_FSETID
> and that should do it.
> 
> This should fix the xfstest generic/375 test case.
> 
> We don't have to switch uid for this to work. That could be one optimization
> that pass a parameter to lo_change_cred() to only switch gid and not uid.
> 
> Also this will not work whenever (if ever) we support idmapped mounts. In
> that case it is possible that uid/gid in request are 0/0 but still we
> need to clear SGID. So we will have to pick a non-root sgid and switch
> to that instead. That's an TODO item for future when idmapped mount
> support is introduced.
> 
> This patch only adds the capability to switch creds and drop FSETID
> when acl xattr is set. This does not take affect yet. It can take
> affect when next patch adds the capability to enable posix_acl.
> 
> Reported-by: Luis Henriques <lhenriques@suse.de>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  tools/virtiofsd/passthrough_ll.c | 75 ++++++++++++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 0c9084ea15..113c725def 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -175,6 +175,7 @@ struct lo_data {
>      int user_killpriv_v2, killpriv_v2;
>      /* If set, virtiofsd is responsible for setting umask during creation */
>      bool change_umask;
> +    int posix_acl;
>  };
>  
>  static const struct fuse_opt lo_opts[] = {
> @@ -1185,6 +1186,51 @@ static void lo_restore_cred(struct lo_cred *old, bool restore_umask)
>          umask(old->umask);
>  }
>  
> +/*
> + * A helper to change cred and drop capability. Returns 0 on success and
> + * errno on error
> + */
> +static int lo_drop_cap_change_cred(fuse_req_t req, struct lo_cred *old,
> +                                   bool change_umask, const char *cap_name,
> +                                   bool *cap_dropped)
> +{
> +    int ret;
> +    bool __cap_dropped;
> +
> +    assert(cap_name);
> +
> +    ret = drop_effective_cap(cap_name, &__cap_dropped);
> +    if (ret) {
> +        return ret;
> +    }
> +
> +    ret = lo_change_cred(req, old, change_umask);
> +    if (ret) {
> +        if (__cap_dropped) {
> +            if (gain_effective_cap(cap_name)) {
> +                fuse_log(FUSE_LOG_ERR, "Failed to gain CAP_%s\n", cap_name);
> +            }
> +        }
> +    }
> +
> +    if (cap_dropped) {
> +        *cap_dropped = __cap_dropped;
> +    }
> +    return ret;
> +}
> +
> +static void lo_restore_cred_gain_cap(struct lo_cred *old, bool restore_umask,
> +                                     const char *cap_name)
> +{
> +    assert(cap_name);
> +
> +    lo_restore_cred(old, restore_umask);
> +
> +    if (gain_effective_cap(cap_name)) {
> +        fuse_log(FUSE_LOG_ERR, "Failed to gain CAP_%s\n", cap_name);
> +    }
> +}
> +
>  static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent,
>                               const char *name, mode_t mode, dev_t rdev,
>                               const char *link)
> @@ -2976,6 +3022,9 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
>      ssize_t ret;
>      int saverr;
>      int fd = -1;
> +    bool switched_creds = false;
> +    bool cap_fsetid_dropped = false;
> +    struct lo_cred old = {};
>  
>      mapped_name = NULL;
>      name = in_name;
> @@ -3006,6 +3055,26 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
>               ", name=%s value=%s size=%zd)\n", ino, name, value, size);
>  
>      sprintf(procname, "%i", inode->fd);
> +    /*
> +     * If we are setting posix access acl and if SGID needs to be
> +     * cleared, then switch to caller's gid and drop CAP_FSETID
> +     * and that should make sure host kernel clears SGID.
> +     *
> +     * This probably will not work when we support idmapped mounts.
> +     * In that case we will need to find a non-root gid and switch
> +     * to it. (Instead of gid in request). Fix it when we support
> +     * idmapped mounts.
> +     */
> +    if (lo->posix_acl && !strcmp(name, "system.posix_acl_access")
> +        && (extra_flags & FUSE_SETXATTR_ACL_KILL_SGID)) {
> +        ret = lo_drop_cap_change_cred(req, &old, false, "FSETID",
> +                                      &cap_fsetid_dropped);
> +        if (ret) {
> +            saverr = ret;
> +            goto out;
> +        }
> +        switched_creds = true;
> +    }
>      if (S_ISREG(inode->filetype) || S_ISDIR(inode->filetype)) {
>          fd = openat(lo->proc_self_fd, procname, O_RDONLY);
>          if (fd < 0) {
> @@ -3021,6 +3090,12 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
>          saverr = ret == -1 ? errno : 0;
>          FCHDIR_NOFAIL(lo->root.fd);
>      }
> +    if (switched_creds) {
> +        if (cap_fsetid_dropped)
> +            lo_restore_cred_gain_cap(&old, false, "FSETID");
> +        else
> +            lo_restore_cred(&old, false);
> +    }
>  
>  out:
>      if (fd >= 0) {
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v7 6/7] virtiofsd: Switch creds, drop FSETID for system.posix_acl_access xattr

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> When posix access acls are set on a file, it can lead to adjusting file
> permissions (mode) as well. If caller does not have CAP_FSETID and it
> also does not have membership of owner group, this will lead to clearing
> SGID bit in mode.
> 
> Current fuse code is written in such a way that it expects file server
> to take care of chaning file mode (permission), if there is a need.
> Right now, host kernel does not clear SGID bit because virtiofsd is
> running as root and has CAP_FSETID. For host kernel to clear SGID,
> virtiofsd need to switch to gid of caller in guest and also drop
> CAP_FSETID (if caller did not have it to begin with).
> 
> If SGID needs to be cleared, client will set the flag
> FUSE_SETXATTR_ACL_KILL_SGID in setxattr request. In that case server
> should kill sgid.
> 
> Currently just switch to uid/gid of the caller and drop CAP_FSETID
> and that should do it.
> 
> This should fix the xfstest generic/375 test case.
> 
> We don't have to switch uid for this to work. That could be one optimization
> that pass a parameter to lo_change_cred() to only switch gid and not uid.
> 
> Also this will not work whenever (if ever) we support idmapped mounts. In
> that case it is possible that uid/gid in request are 0/0 but still we
> need to clear SGID. So we will have to pick a non-root sgid and switch
> to that instead. That's an TODO item for future when idmapped mount
> support is introduced.
> 
> This patch only adds the capability to switch creds and drop FSETID
> when acl xattr is set. This does not take affect yet. It can take
> affect when next patch adds the capability to enable posix_acl.
> 
> Reported-by: Luis Henriques <lhenriques@suse.de>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  tools/virtiofsd/passthrough_ll.c | 75 ++++++++++++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 0c9084ea15..113c725def 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -175,6 +175,7 @@ struct lo_data {
>      int user_killpriv_v2, killpriv_v2;
>      /* If set, virtiofsd is responsible for setting umask during creation */
>      bool change_umask;
> +    int posix_acl;
>  };
>  
>  static const struct fuse_opt lo_opts[] = {
> @@ -1185,6 +1186,51 @@ static void lo_restore_cred(struct lo_cred *old, bool restore_umask)
>          umask(old->umask);
>  }
>  
> +/*
> + * A helper to change cred and drop capability. Returns 0 on success and
> + * errno on error
> + */
> +static int lo_drop_cap_change_cred(fuse_req_t req, struct lo_cred *old,
> +                                   bool change_umask, const char *cap_name,
> +                                   bool *cap_dropped)
> +{
> +    int ret;
> +    bool __cap_dropped;
> +
> +    assert(cap_name);
> +
> +    ret = drop_effective_cap(cap_name, &__cap_dropped);
> +    if (ret) {
> +        return ret;
> +    }
> +
> +    ret = lo_change_cred(req, old, change_umask);
> +    if (ret) {
> +        if (__cap_dropped) {
> +            if (gain_effective_cap(cap_name)) {
> +                fuse_log(FUSE_LOG_ERR, "Failed to gain CAP_%s\n", cap_name);
> +            }
> +        }
> +    }
> +
> +    if (cap_dropped) {
> +        *cap_dropped = __cap_dropped;
> +    }
> +    return ret;
> +}
> +
> +static void lo_restore_cred_gain_cap(struct lo_cred *old, bool restore_umask,
> +                                     const char *cap_name)
> +{
> +    assert(cap_name);
> +
> +    lo_restore_cred(old, restore_umask);
> +
> +    if (gain_effective_cap(cap_name)) {
> +        fuse_log(FUSE_LOG_ERR, "Failed to gain CAP_%s\n", cap_name);
> +    }
> +}
> +
>  static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent,
>                               const char *name, mode_t mode, dev_t rdev,
>                               const char *link)
> @@ -2976,6 +3022,9 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
>      ssize_t ret;
>      int saverr;
>      int fd = -1;
> +    bool switched_creds = false;
> +    bool cap_fsetid_dropped = false;
> +    struct lo_cred old = {};
>  
>      mapped_name = NULL;
>      name = in_name;
> @@ -3006,6 +3055,26 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
>               ", name=%s value=%s size=%zd)\n", ino, name, value, size);
>  
>      sprintf(procname, "%i", inode->fd);
> +    /*
> +     * If we are setting posix access acl and if SGID needs to be
> +     * cleared, then switch to caller's gid and drop CAP_FSETID
> +     * and that should make sure host kernel clears SGID.
> +     *
> +     * This probably will not work when we support idmapped mounts.
> +     * In that case we will need to find a non-root gid and switch
> +     * to it. (Instead of gid in request). Fix it when we support
> +     * idmapped mounts.
> +     */
> +    if (lo->posix_acl && !strcmp(name, "system.posix_acl_access")
> +        && (extra_flags & FUSE_SETXATTR_ACL_KILL_SGID)) {
> +        ret = lo_drop_cap_change_cred(req, &old, false, "FSETID",
> +                                      &cap_fsetid_dropped);
> +        if (ret) {
> +            saverr = ret;
> +            goto out;
> +        }
> +        switched_creds = true;
> +    }
>      if (S_ISREG(inode->filetype) || S_ISDIR(inode->filetype)) {
>          fd = openat(lo->proc_self_fd, procname, O_RDONLY);
>          if (fd < 0) {
> @@ -3021,6 +3090,12 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
>          saverr = ret == -1 ? errno : 0;
>          FCHDIR_NOFAIL(lo->root.fd);
>      }
> +    if (switched_creds) {
> +        if (cap_fsetid_dropped)
> +            lo_restore_cred_gain_cap(&old, false, "FSETID");
> +        else
> +            lo_restore_cred(&old, false);
> +    }
>  
>  out:
>      if (fd >= 0) {
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH v7 6/7] virtiofsd: Switch creds, drop FSETID for system.posix_acl_access xattr

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> When posix access acls are set on a file, it can lead to adjusting file
> permissions (mode) as well. If caller does not have CAP_FSETID and it
> also does not have membership of owner group, this will lead to clearing
> SGID bit in mode.
> 
> Current fuse code is written in such a way that it expects file server
> to take care of chaning file mode (permission), if there is a need.
> Right now, host kernel does not clear SGID bit because virtiofsd is
> running as root and has CAP_FSETID. For host kernel to clear SGID,
> virtiofsd need to switch to gid of caller in guest and also drop
> CAP_FSETID (if caller did not have it to begin with).
> 
> If SGID needs to be cleared, client will set the flag
> FUSE_SETXATTR_ACL_KILL_SGID in setxattr request. In that case server
> should kill sgid.
> 
> Currently just switch to uid/gid of the caller and drop CAP_FSETID
> and that should do it.
> 
> This should fix the xfstest generic/375 test case.
> 
> We don't have to switch uid for this to work. That could be one optimization
> that pass a parameter to lo_change_cred() to only switch gid and not uid.
> 
> Also this will not work whenever (if ever) we support idmapped mounts. In
> that case it is possible that uid/gid in request are 0/0 but still we
> need to clear SGID. So we will have to pick a non-root sgid and switch
> to that instead. That's an TODO item for future when idmapped mount
> support is introduced.
> 
> This patch only adds the capability to switch creds and drop FSETID
> when acl xattr is set. This does not take affect yet. It can take
> affect when next patch adds the capability to enable posix_acl.
> 
> Reported-by: Luis Henriques <lhenriques@suse.de>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  tools/virtiofsd/passthrough_ll.c | 75 ++++++++++++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 0c9084ea15..113c725def 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -175,6 +175,7 @@ struct lo_data {
>      int user_killpriv_v2, killpriv_v2;
>      /* If set, virtiofsd is responsible for setting umask during creation */
>      bool change_umask;
> +    int posix_acl;
>  };
>  
>  static const struct fuse_opt lo_opts[] = {
> @@ -1185,6 +1186,51 @@ static void lo_restore_cred(struct lo_cred *old, bool restore_umask)
>          umask(old->umask);
>  }
>  
> +/*
> + * A helper to change cred and drop capability. Returns 0 on success and
> + * errno on error
> + */
> +static int lo_drop_cap_change_cred(fuse_req_t req, struct lo_cred *old,
> +                                   bool change_umask, const char *cap_name,
> +                                   bool *cap_dropped)
> +{
> +    int ret;
> +    bool __cap_dropped;
> +
> +    assert(cap_name);
> +
> +    ret = drop_effective_cap(cap_name, &__cap_dropped);
> +    if (ret) {
> +        return ret;
> +    }
> +
> +    ret = lo_change_cred(req, old, change_umask);
> +    if (ret) {
> +        if (__cap_dropped) {
> +            if (gain_effective_cap(cap_name)) {
> +                fuse_log(FUSE_LOG_ERR, "Failed to gain CAP_%s\n", cap_name);
> +            }
> +        }
> +    }
> +
> +    if (cap_dropped) {
> +        *cap_dropped = __cap_dropped;
> +    }
> +    return ret;
> +}
> +
> +static void lo_restore_cred_gain_cap(struct lo_cred *old, bool restore_umask,
> +                                     const char *cap_name)
> +{
> +    assert(cap_name);
> +
> +    lo_restore_cred(old, restore_umask);
> +
> +    if (gain_effective_cap(cap_name)) {
> +        fuse_log(FUSE_LOG_ERR, "Failed to gain CAP_%s\n", cap_name);
> +    }
> +}
> +
>  static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent,
>                               const char *name, mode_t mode, dev_t rdev,
>                               const char *link)
> @@ -2976,6 +3022,9 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
>      ssize_t ret;
>      int saverr;
>      int fd = -1;
> +    bool switched_creds = false;
> +    bool cap_fsetid_dropped = false;
> +    struct lo_cred old = {};
>  
>      mapped_name = NULL;
>      name = in_name;
> @@ -3006,6 +3055,26 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
>               ", name=%s value=%s size=%zd)\n", ino, name, value, size);
>  
>      sprintf(procname, "%i", inode->fd);
> +    /*
> +     * If we are setting posix access acl and if SGID needs to be
> +     * cleared, then switch to caller's gid and drop CAP_FSETID
> +     * and that should make sure host kernel clears SGID.
> +     *
> +     * This probably will not work when we support idmapped mounts.
> +     * In that case we will need to find a non-root gid and switch
> +     * to it. (Instead of gid in request). Fix it when we support
> +     * idmapped mounts.
> +     */
> +    if (lo->posix_acl && !strcmp(name, "system.posix_acl_access")
> +        && (extra_flags & FUSE_SETXATTR_ACL_KILL_SGID)) {
> +        ret = lo_drop_cap_change_cred(req, &old, false, "FSETID",
> +                                      &cap_fsetid_dropped);
> +        if (ret) {
> +            saverr = ret;
> +            goto out;
> +        }
> +        switched_creds = true;
> +    }
>      if (S_ISREG(inode->filetype) || S_ISDIR(inode->filetype)) {
>          fd = openat(lo->proc_self_fd, procname, O_RDONLY);
>          if (fd < 0) {
> @@ -3021,6 +3090,12 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
>          saverr = ret == -1 ? errno : 0;
>          FCHDIR_NOFAIL(lo->root.fd);
>      }
> +    if (switched_creds) {
> +        if (cap_fsetid_dropped)
> +            lo_restore_cred_gain_cap(&old, false, "FSETID");
> +        else
> +            lo_restore_cred(&old, false);
> +    }
>  
>  out:
>      if (fd >= 0) {
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v7 5/7] virtiofsd: Add capability to change/restore umask

[Vivek Goyal]

On Mon, Jun 28, 2021 at 05:12:13PM +0100, Dr. David Alan Gilbert wrote:
> * Vivek Goyal (vgoyal@redhat.com) wrote:
> > 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 few patches 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 9f5cd98fb5..0c9084ea15 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -122,6 +122,7 @@ struct lo_inode {
> >  struct lo_cred {
> >      uid_t euid;
> >      gid_t egid;
> > +    mode_t umask;
> >  };
> >  
> >  enum {
> > @@ -172,6 +173,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[] = {
> > @@ -1134,7 +1137,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;
> >  
> > @@ -1154,11 +1158,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;
> >  
> > @@ -1173,6 +1180,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,
> > @@ -1202,7 +1212,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));
> 
> Can you explain what these ISLNK checks are for (insid mknod_symlink, so
> is that always true or irrelevant?)

I think I put this check in because if we are creating symlink then we
don't have to change umask as symlink will always get a some fix
mode (usually 777) and umask will not have an affect. So this is
just an optimization to avoid switching umask in some cases. I 
can't think of any other reason.

thanks
Vivek

Re: [Virtio-fs] [PATCH v7 5/7] virtiofsd: Add capability to change/restore umask

[Vivek Goyal]

On Mon, Jun 28, 2021 at 05:12:13PM +0100, Dr. David Alan Gilbert wrote:
> * Vivek Goyal (vgoyal@redhat.com) wrote:
> > 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 few patches 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 9f5cd98fb5..0c9084ea15 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -122,6 +122,7 @@ struct lo_inode {
> >  struct lo_cred {
> >      uid_t euid;
> >      gid_t egid;
> > +    mode_t umask;
> >  };
> >  
> >  enum {
> > @@ -172,6 +173,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[] = {
> > @@ -1134,7 +1137,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;
> >  
> > @@ -1154,11 +1158,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;
> >  
> > @@ -1173,6 +1180,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,
> > @@ -1202,7 +1212,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));
> 
> Can you explain what these ISLNK checks are for (insid mknod_symlink, so
> is that always true or irrelevant?)

I think I put this check in because if we are creating symlink then we
don't have to change umask as symlink will always get a some fix
mode (usually 777) and umask will not have an affect. So this is
just an optimization to avoid switching umask in some cases. I 
can't think of any other reason.

thanks
Vivek

Re: [PATCH v7 7/7] virtiofsd: Add an option to enable/disable posix acls

[Dr. David Alan Gilbert]

* 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. 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>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

(I'd have some sympathy to making get silently hide it)

> ---
>  docs/tools/virtiofsd.rst         |   3 +
>  tools/virtiofsd/helper.c         |   1 +
>  tools/virtiofsd/passthrough_ll.c | 115 ++++++++++++++++++++++++++++++-
>  3 files changed, 118 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/tools/virtiofsd.rst b/docs/tools/virtiofsd.rst
> index 00554c75bd..a41f934999 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 5e98ed702b..a8295d975a 100644
> --- a/tools/virtiofsd/helper.c
> +++ b/tools/virtiofsd/helper.c
> @@ -186,6 +186,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 113c725def..e80fd76d71 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -175,7 +175,7 @@ struct lo_data {
>      int user_killpriv_v2, killpriv_v2;
>      /* If set, virtiofsd is responsible for setting umask during creation */
>      bool change_umask;
> -    int posix_acl;
> +    int user_posix_acl, posix_acl;
>  };
>  
>  static const struct fuse_opt lo_opts[] = {
> @@ -208,6 +208,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;
> @@ -706,6 +708,32 @@ 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, print error message
> +         * now. It will fail later in fuse_lowlevel.c
> +         */
> +        if (!(conn->capable & FUSE_CAP_POSIX_ACL) ||
> +            !(conn->capable & FUSE_CAP_DONT_MASK) ||
> +            !(conn->capable & FUSE_CAP_SETXATTR_EXT)) {
> +            fuse_log(FUSE_LOG_ERR, "lo_init: Can not enable posix acl."
> +                     " kernel does not support FUSE_POSIX_ACL, FUSE_DONT_MASK"
> +                     " or FUSE_SETXATTR_EXT capability.\n");
> +        } else {
> +            fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling posix acl\n");
> +        }
> +
> +        conn->want |= FUSE_CAP_POSIX_ACL | FUSE_CAP_DONT_MASK |
> +                      FUSE_CAP_SETXATTR_EXT;
> +        lo->change_umask = true;
> +        lo->posix_acl = 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,
> @@ -2783,6 +2811,63 @@ static int xattr_map_server(const struct lo_data *lo, const char *server_name,
>          assert(fchdir_res == 0);                       \
>      } while (0)
>  
> +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)
>  {
> @@ -2796,6 +2881,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) {
> @@ -2986,6 +3076,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 {
>          /*
> @@ -3026,6 +3122,11 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
>      bool cap_fsetid_dropped = false;
>      struct lo_cred old = {};
>  
> +    if (block_xattr(lo, in_name)) {
> +        fuse_reply_err(req, EOPNOTSUPP);
> +        return;
> +    }
> +
>      mapped_name = NULL;
>      name = in_name;
>      if (lo->xattrmap) {
> @@ -3118,6 +3219,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) {
> @@ -3812,6 +3918,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;
> @@ -3940,6 +4047,12 @@ 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
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Dr. David Alan Gilbert]

* 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. 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>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

(I'd have some sympathy to making get silently hide it)

> ---
>  docs/tools/virtiofsd.rst         |   3 +
>  tools/virtiofsd/helper.c         |   1 +
>  tools/virtiofsd/passthrough_ll.c | 115 ++++++++++++++++++++++++++++++-
>  3 files changed, 118 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/tools/virtiofsd.rst b/docs/tools/virtiofsd.rst
> index 00554c75bd..a41f934999 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 5e98ed702b..a8295d975a 100644
> --- a/tools/virtiofsd/helper.c
> +++ b/tools/virtiofsd/helper.c
> @@ -186,6 +186,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 113c725def..e80fd76d71 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -175,7 +175,7 @@ struct lo_data {
>      int user_killpriv_v2, killpriv_v2;
>      /* If set, virtiofsd is responsible for setting umask during creation */
>      bool change_umask;
> -    int posix_acl;
> +    int user_posix_acl, posix_acl;
>  };
>  
>  static const struct fuse_opt lo_opts[] = {
> @@ -208,6 +208,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;
> @@ -706,6 +708,32 @@ 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, print error message
> +         * now. It will fail later in fuse_lowlevel.c
> +         */
> +        if (!(conn->capable & FUSE_CAP_POSIX_ACL) ||
> +            !(conn->capable & FUSE_CAP_DONT_MASK) ||
> +            !(conn->capable & FUSE_CAP_SETXATTR_EXT)) {
> +            fuse_log(FUSE_LOG_ERR, "lo_init: Can not enable posix acl."
> +                     " kernel does not support FUSE_POSIX_ACL, FUSE_DONT_MASK"
> +                     " or FUSE_SETXATTR_EXT capability.\n");
> +        } else {
> +            fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling posix acl\n");
> +        }
> +
> +        conn->want |= FUSE_CAP_POSIX_ACL | FUSE_CAP_DONT_MASK |
> +                      FUSE_CAP_SETXATTR_EXT;
> +        lo->change_umask = true;
> +        lo->posix_acl = 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,
> @@ -2783,6 +2811,63 @@ static int xattr_map_server(const struct lo_data *lo, const char *server_name,
>          assert(fchdir_res == 0);                       \
>      } while (0)
>  
> +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)
>  {
> @@ -2796,6 +2881,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) {
> @@ -2986,6 +3076,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 {
>          /*
> @@ -3026,6 +3122,11 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
>      bool cap_fsetid_dropped = false;
>      struct lo_cred old = {};
>  
> +    if (block_xattr(lo, in_name)) {
> +        fuse_reply_err(req, EOPNOTSUPP);
> +        return;
> +    }
> +
>      mapped_name = NULL;
>      name = in_name;
>      if (lo->xattrmap) {
> @@ -3118,6 +3219,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) {
> @@ -3812,6 +3918,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;
> @@ -3940,6 +4047,12 @@ 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
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v7 3/7] virtiofsd: Add support for extended setxattr

[Vivek Goyal]

On Mon, Jun 28, 2021 at 04:49:02PM +0100, Dr. David Alan Gilbert wrote:
> * Vivek Goyal (vgoyal@redhat.com) wrote:
> > Add the bits to enable support for setxattr_ext if fuse offers it. Do not
> > enable it by default yet. Let passthrough_ll opt-in. Enabling it by deafult
> > kind of automatically means that you are taking responsibility of clearing
> > SGID if ACL is set.
> > 
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  tools/virtiofsd/fuse_common.h    |  5 +++++
> >  tools/virtiofsd/fuse_lowlevel.c  | 11 ++++++++++-
> >  tools/virtiofsd/fuse_lowlevel.h  |  3 ++-
> >  tools/virtiofsd/passthrough_ll.c |  3 ++-
> >  4 files changed, 19 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/virtiofsd/fuse_common.h b/tools/virtiofsd/fuse_common.h
> > index 0c2665b977..8abac80098 100644
> > --- a/tools/virtiofsd/fuse_common.h
> > +++ b/tools/virtiofsd/fuse_common.h
> > @@ -377,6 +377,11 @@ struct fuse_file_info {
> >   */
> >  #define FUSE_CAP_SETXATTR_EXT (1 << 29)
> >  
> > +/**
> > + * Indicates that file server supports extended struct fuse_setxattr_in
> > + */
> > +#define FUSE_CAP_SETXATTR_EXT (1 << 29)
> > +
> 
> You already added that in 1/7 - but other than that, 

Good catch. Do I need to repost patch/series due to this change or this
is something you can take care of while applying patches.

Vivek

> 
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> >  /**
> >   * Ioctl flags
> >   *
> > diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
> > index c2b6ff1686..d1e24c013f 100644
> > --- a/tools/virtiofsd/fuse_lowlevel.c
> > +++ b/tools/virtiofsd/fuse_lowlevel.c
> > @@ -1439,7 +1439,9 @@ static void do_setxattr(fuse_req_t req, fuse_ino_t nodeid,
> >      }
> >  
> >      if (req->se->op.setxattr) {
> > -        req->se->op.setxattr(req, nodeid, name, value, arg->size, arg->flags);
> > +        uint32_t setxattr_flags = setxattr_ext ? arg->setxattr_flags : 0;
> > +        req->se->op.setxattr(req, nodeid, name, value, arg->size, arg->flags,
> > +                             setxattr_flags);
> >      } else {
> >          fuse_reply_err(req, ENOSYS);
> >      }
> > @@ -1986,6 +1988,9 @@ static void do_init(fuse_req_t req, fuse_ino_t nodeid,
> >      if (arg->flags & FUSE_HANDLE_KILLPRIV_V2) {
> >          se->conn.capable |= FUSE_CAP_HANDLE_KILLPRIV_V2;
> >      }
> > +    if (arg->flags & FUSE_SETXATTR_EXT) {
> > +        se->conn.capable |= FUSE_CAP_SETXATTR_EXT;
> > +    }
> >  #ifdef HAVE_SPLICE
> >  #ifdef HAVE_VMSPLICE
> >      se->conn.capable |= FUSE_CAP_SPLICE_WRITE | FUSE_CAP_SPLICE_MOVE;
> > @@ -2121,6 +2126,10 @@ static void do_init(fuse_req_t req, fuse_ino_t nodeid,
> >          outarg.flags |= FUSE_HANDLE_KILLPRIV_V2;
> >      }
> >  
> > +    if (se->conn.want & FUSE_CAP_SETXATTR_EXT) {
> > +        outarg.flags |= FUSE_SETXATTR_EXT;
> > +    }
> > +
> >      fuse_log(FUSE_LOG_DEBUG, "   INIT: %u.%u\n", outarg.major, outarg.minor);
> >      fuse_log(FUSE_LOG_DEBUG, "   flags=0x%08x\n", outarg.flags);
> >      fuse_log(FUSE_LOG_DEBUG, "   max_readahead=0x%08x\n", outarg.max_readahead);
> > diff --git a/tools/virtiofsd/fuse_lowlevel.h b/tools/virtiofsd/fuse_lowlevel.h
> > index 3bf786b034..4b4e8c9724 100644
> > --- a/tools/virtiofsd/fuse_lowlevel.h
> > +++ b/tools/virtiofsd/fuse_lowlevel.h
> > @@ -798,7 +798,8 @@ struct fuse_lowlevel_ops {
> >       *   fuse_reply_err
> >       */
> >      void (*setxattr)(fuse_req_t req, fuse_ino_t ino, const char *name,
> > -                     const char *value, size_t size, int flags);
> > +                     const char *value, size_t size, int flags,
> > +                     uint32_t setxattr_flags);
> >  
> >      /**
> >       * Get an extended attribute
> > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > index ec91b3c133..9f5cd98fb5 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -2955,7 +2955,8 @@ out:
> >  }
> >  
> >  static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
> > -                        const char *value, size_t size, int flags)
> > +                        const char *value, size_t size, int flags,
> > +                        uint32_t extra_flags)
> >  {
> >      char procname[64];
> >      const char *name;
> > -- 
> > 2.25.4
> > 
> -- 
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

Re: [Virtio-fs] [PATCH v7 3/7] virtiofsd: Add support for extended setxattr

[Vivek Goyal]

On Mon, Jun 28, 2021 at 04:49:02PM +0100, Dr. David Alan Gilbert wrote:
> * Vivek Goyal (vgoyal@redhat.com) wrote:
> > Add the bits to enable support for setxattr_ext if fuse offers it. Do not
> > enable it by default yet. Let passthrough_ll opt-in. Enabling it by deafult
> > kind of automatically means that you are taking responsibility of clearing
> > SGID if ACL is set.
> > 
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  tools/virtiofsd/fuse_common.h    |  5 +++++
> >  tools/virtiofsd/fuse_lowlevel.c  | 11 ++++++++++-
> >  tools/virtiofsd/fuse_lowlevel.h  |  3 ++-
> >  tools/virtiofsd/passthrough_ll.c |  3 ++-
> >  4 files changed, 19 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/virtiofsd/fuse_common.h b/tools/virtiofsd/fuse_common.h
> > index 0c2665b977..8abac80098 100644
> > --- a/tools/virtiofsd/fuse_common.h
> > +++ b/tools/virtiofsd/fuse_common.h
> > @@ -377,6 +377,11 @@ struct fuse_file_info {
> >   */
> >  #define FUSE_CAP_SETXATTR_EXT (1 << 29)
> >  
> > +/**
> > + * Indicates that file server supports extended struct fuse_setxattr_in
> > + */
> > +#define FUSE_CAP_SETXATTR_EXT (1 << 29)
> > +
> 
> You already added that in 1/7 - but other than that, 

Good catch. Do I need to repost patch/series due to this change or this
is something you can take care of while applying patches.

Vivek

> 
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> >  /**
> >   * Ioctl flags
> >   *
> > diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
> > index c2b6ff1686..d1e24c013f 100644
> > --- a/tools/virtiofsd/fuse_lowlevel.c
> > +++ b/tools/virtiofsd/fuse_lowlevel.c
> > @@ -1439,7 +1439,9 @@ static void do_setxattr(fuse_req_t req, fuse_ino_t nodeid,
> >      }
> >  
> >      if (req->se->op.setxattr) {
> > -        req->se->op.setxattr(req, nodeid, name, value, arg->size, arg->flags);
> > +        uint32_t setxattr_flags = setxattr_ext ? arg->setxattr_flags : 0;
> > +        req->se->op.setxattr(req, nodeid, name, value, arg->size, arg->flags,
> > +                             setxattr_flags);
> >      } else {
> >          fuse_reply_err(req, ENOSYS);
> >      }
> > @@ -1986,6 +1988,9 @@ static void do_init(fuse_req_t req, fuse_ino_t nodeid,
> >      if (arg->flags & FUSE_HANDLE_KILLPRIV_V2) {
> >          se->conn.capable |= FUSE_CAP_HANDLE_KILLPRIV_V2;
> >      }
> > +    if (arg->flags & FUSE_SETXATTR_EXT) {
> > +        se->conn.capable |= FUSE_CAP_SETXATTR_EXT;
> > +    }
> >  #ifdef HAVE_SPLICE
> >  #ifdef HAVE_VMSPLICE
> >      se->conn.capable |= FUSE_CAP_SPLICE_WRITE | FUSE_CAP_SPLICE_MOVE;
> > @@ -2121,6 +2126,10 @@ static void do_init(fuse_req_t req, fuse_ino_t nodeid,
> >          outarg.flags |= FUSE_HANDLE_KILLPRIV_V2;
> >      }
> >  
> > +    if (se->conn.want & FUSE_CAP_SETXATTR_EXT) {
> > +        outarg.flags |= FUSE_SETXATTR_EXT;
> > +    }
> > +
> >      fuse_log(FUSE_LOG_DEBUG, "   INIT: %u.%u\n", outarg.major, outarg.minor);
> >      fuse_log(FUSE_LOG_DEBUG, "   flags=0x%08x\n", outarg.flags);
> >      fuse_log(FUSE_LOG_DEBUG, "   max_readahead=0x%08x\n", outarg.max_readahead);
> > diff --git a/tools/virtiofsd/fuse_lowlevel.h b/tools/virtiofsd/fuse_lowlevel.h
> > index 3bf786b034..4b4e8c9724 100644
> > --- a/tools/virtiofsd/fuse_lowlevel.h
> > +++ b/tools/virtiofsd/fuse_lowlevel.h
> > @@ -798,7 +798,8 @@ struct fuse_lowlevel_ops {
> >       *   fuse_reply_err
> >       */
> >      void (*setxattr)(fuse_req_t req, fuse_ino_t ino, const char *name,
> > -                     const char *value, size_t size, int flags);
> > +                     const char *value, size_t size, int flags,
> > +                     uint32_t setxattr_flags);
> >  
> >      /**
> >       * Get an extended attribute
> > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > index ec91b3c133..9f5cd98fb5 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -2955,7 +2955,8 @@ out:
> >  }
> >  
> >  static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
> > -                        const char *value, size_t size, int flags)
> > +                        const char *value, size_t size, int flags,
> > +                        uint32_t extra_flags)
> >  {
> >      char procname[64];
> >      const char *name;
> > -- 
> > 2.25.4
> > 
> -- 
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

Re: [PATCH v7 3/7] virtiofsd: Add support for extended setxattr

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> On Mon, Jun 28, 2021 at 04:49:02PM +0100, Dr. David Alan Gilbert wrote:
> > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > Add the bits to enable support for setxattr_ext if fuse offers it. Do not
> > > enable it by default yet. Let passthrough_ll opt-in. Enabling it by deafult
> > > kind of automatically means that you are taking responsibility of clearing
> > > SGID if ACL is set.
> > > 
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > ---
> > >  tools/virtiofsd/fuse_common.h    |  5 +++++
> > >  tools/virtiofsd/fuse_lowlevel.c  | 11 ++++++++++-
> > >  tools/virtiofsd/fuse_lowlevel.h  |  3 ++-
> > >  tools/virtiofsd/passthrough_ll.c |  3 ++-
> > >  4 files changed, 19 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/tools/virtiofsd/fuse_common.h b/tools/virtiofsd/fuse_common.h
> > > index 0c2665b977..8abac80098 100644
> > > --- a/tools/virtiofsd/fuse_common.h
> > > +++ b/tools/virtiofsd/fuse_common.h
> > > @@ -377,6 +377,11 @@ struct fuse_file_info {
> > >   */
> > >  #define FUSE_CAP_SETXATTR_EXT (1 << 29)
> > >  
> > > +/**
> > > + * Indicates that file server supports extended struct fuse_setxattr_in
> > > + */
> > > +#define FUSE_CAP_SETXATTR_EXT (1 << 29)
> > > +
> > 
> > You already added that in 1/7 - but other than that, 
> 
> Good catch. Do I need to repost patch/series due to this change or this
> is something you can take care of while applying patches.

I can fix it.

Dave

> Vivek
> 
> > 
> > 
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > 
> > >  /**
> > >   * Ioctl flags
> > >   *
> > > diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
> > > index c2b6ff1686..d1e24c013f 100644
> > > --- a/tools/virtiofsd/fuse_lowlevel.c
> > > +++ b/tools/virtiofsd/fuse_lowlevel.c
> > > @@ -1439,7 +1439,9 @@ static void do_setxattr(fuse_req_t req, fuse_ino_t nodeid,
> > >      }
> > >  
> > >      if (req->se->op.setxattr) {
> > > -        req->se->op.setxattr(req, nodeid, name, value, arg->size, arg->flags);
> > > +        uint32_t setxattr_flags = setxattr_ext ? arg->setxattr_flags : 0;
> > > +        req->se->op.setxattr(req, nodeid, name, value, arg->size, arg->flags,
> > > +                             setxattr_flags);
> > >      } else {
> > >          fuse_reply_err(req, ENOSYS);
> > >      }
> > > @@ -1986,6 +1988,9 @@ static void do_init(fuse_req_t req, fuse_ino_t nodeid,
> > >      if (arg->flags & FUSE_HANDLE_KILLPRIV_V2) {
> > >          se->conn.capable |= FUSE_CAP_HANDLE_KILLPRIV_V2;
> > >      }
> > > +    if (arg->flags & FUSE_SETXATTR_EXT) {
> > > +        se->conn.capable |= FUSE_CAP_SETXATTR_EXT;
> > > +    }
> > >  #ifdef HAVE_SPLICE
> > >  #ifdef HAVE_VMSPLICE
> > >      se->conn.capable |= FUSE_CAP_SPLICE_WRITE | FUSE_CAP_SPLICE_MOVE;
> > > @@ -2121,6 +2126,10 @@ static void do_init(fuse_req_t req, fuse_ino_t nodeid,
> > >          outarg.flags |= FUSE_HANDLE_KILLPRIV_V2;
> > >      }
> > >  
> > > +    if (se->conn.want & FUSE_CAP_SETXATTR_EXT) {
> > > +        outarg.flags |= FUSE_SETXATTR_EXT;
> > > +    }
> > > +
> > >      fuse_log(FUSE_LOG_DEBUG, "   INIT: %u.%u\n", outarg.major, outarg.minor);
> > >      fuse_log(FUSE_LOG_DEBUG, "   flags=0x%08x\n", outarg.flags);
> > >      fuse_log(FUSE_LOG_DEBUG, "   max_readahead=0x%08x\n", outarg.max_readahead);
> > > diff --git a/tools/virtiofsd/fuse_lowlevel.h b/tools/virtiofsd/fuse_lowlevel.h
> > > index 3bf786b034..4b4e8c9724 100644
> > > --- a/tools/virtiofsd/fuse_lowlevel.h
> > > +++ b/tools/virtiofsd/fuse_lowlevel.h
> > > @@ -798,7 +798,8 @@ struct fuse_lowlevel_ops {
> > >       *   fuse_reply_err
> > >       */
> > >      void (*setxattr)(fuse_req_t req, fuse_ino_t ino, const char *name,
> > > -                     const char *value, size_t size, int flags);
> > > +                     const char *value, size_t size, int flags,
> > > +                     uint32_t setxattr_flags);
> > >  
> > >      /**
> > >       * Get an extended attribute
> > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > index ec91b3c133..9f5cd98fb5 100644
> > > --- a/tools/virtiofsd/passthrough_ll.c
> > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > @@ -2955,7 +2955,8 @@ out:
> > >  }
> > >  
> > >  static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
> > > -                        const char *value, size_t size, int flags)
> > > +                        const char *value, size_t size, int flags,
> > > +                        uint32_t extra_flags)
> > >  {
> > >      char procname[64];
> > >      const char *name;
> > > -- 
> > > 2.25.4
> > > 
> > -- 
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH v7 3/7] virtiofsd: Add support for extended setxattr

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> On Mon, Jun 28, 2021 at 04:49:02PM +0100, Dr. David Alan Gilbert wrote:
> > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > Add the bits to enable support for setxattr_ext if fuse offers it. Do not
> > > enable it by default yet. Let passthrough_ll opt-in. Enabling it by deafult
> > > kind of automatically means that you are taking responsibility of clearing
> > > SGID if ACL is set.
> > > 
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > ---
> > >  tools/virtiofsd/fuse_common.h    |  5 +++++
> > >  tools/virtiofsd/fuse_lowlevel.c  | 11 ++++++++++-
> > >  tools/virtiofsd/fuse_lowlevel.h  |  3 ++-
> > >  tools/virtiofsd/passthrough_ll.c |  3 ++-
> > >  4 files changed, 19 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/tools/virtiofsd/fuse_common.h b/tools/virtiofsd/fuse_common.h
> > > index 0c2665b977..8abac80098 100644
> > > --- a/tools/virtiofsd/fuse_common.h
> > > +++ b/tools/virtiofsd/fuse_common.h
> > > @@ -377,6 +377,11 @@ struct fuse_file_info {
> > >   */
> > >  #define FUSE_CAP_SETXATTR_EXT (1 << 29)
> > >  
> > > +/**
> > > + * Indicates that file server supports extended struct fuse_setxattr_in
> > > + */
> > > +#define FUSE_CAP_SETXATTR_EXT (1 << 29)
> > > +
> > 
> > You already added that in 1/7 - but other than that, 
> 
> Good catch. Do I need to repost patch/series due to this change or this
> is something you can take care of while applying patches.

I can fix it.

Dave

> Vivek
> 
> > 
> > 
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > 
> > >  /**
> > >   * Ioctl flags
> > >   *
> > > diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
> > > index c2b6ff1686..d1e24c013f 100644
> > > --- a/tools/virtiofsd/fuse_lowlevel.c
> > > +++ b/tools/virtiofsd/fuse_lowlevel.c
> > > @@ -1439,7 +1439,9 @@ static void do_setxattr(fuse_req_t req, fuse_ino_t nodeid,
> > >      }
> > >  
> > >      if (req->se->op.setxattr) {
> > > -        req->se->op.setxattr(req, nodeid, name, value, arg->size, arg->flags);
> > > +        uint32_t setxattr_flags = setxattr_ext ? arg->setxattr_flags : 0;
> > > +        req->se->op.setxattr(req, nodeid, name, value, arg->size, arg->flags,
> > > +                             setxattr_flags);
> > >      } else {
> > >          fuse_reply_err(req, ENOSYS);
> > >      }
> > > @@ -1986,6 +1988,9 @@ static void do_init(fuse_req_t req, fuse_ino_t nodeid,
> > >      if (arg->flags & FUSE_HANDLE_KILLPRIV_V2) {
> > >          se->conn.capable |= FUSE_CAP_HANDLE_KILLPRIV_V2;
> > >      }
> > > +    if (arg->flags & FUSE_SETXATTR_EXT) {
> > > +        se->conn.capable |= FUSE_CAP_SETXATTR_EXT;
> > > +    }
> > >  #ifdef HAVE_SPLICE
> > >  #ifdef HAVE_VMSPLICE
> > >      se->conn.capable |= FUSE_CAP_SPLICE_WRITE | FUSE_CAP_SPLICE_MOVE;
> > > @@ -2121,6 +2126,10 @@ static void do_init(fuse_req_t req, fuse_ino_t nodeid,
> > >          outarg.flags |= FUSE_HANDLE_KILLPRIV_V2;
> > >      }
> > >  
> > > +    if (se->conn.want & FUSE_CAP_SETXATTR_EXT) {
> > > +        outarg.flags |= FUSE_SETXATTR_EXT;
> > > +    }
> > > +
> > >      fuse_log(FUSE_LOG_DEBUG, "   INIT: %u.%u\n", outarg.major, outarg.minor);
> > >      fuse_log(FUSE_LOG_DEBUG, "   flags=0x%08x\n", outarg.flags);
> > >      fuse_log(FUSE_LOG_DEBUG, "   max_readahead=0x%08x\n", outarg.max_readahead);
> > > diff --git a/tools/virtiofsd/fuse_lowlevel.h b/tools/virtiofsd/fuse_lowlevel.h
> > > index 3bf786b034..4b4e8c9724 100644
> > > --- a/tools/virtiofsd/fuse_lowlevel.h
> > > +++ b/tools/virtiofsd/fuse_lowlevel.h
> > > @@ -798,7 +798,8 @@ struct fuse_lowlevel_ops {
> > >       *   fuse_reply_err
> > >       */
> > >      void (*setxattr)(fuse_req_t req, fuse_ino_t ino, const char *name,
> > > -                     const char *value, size_t size, int flags);
> > > +                     const char *value, size_t size, int flags,
> > > +                     uint32_t setxattr_flags);
> > >  
> > >      /**
> > >       * Get an extended attribute
> > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > index ec91b3c133..9f5cd98fb5 100644
> > > --- a/tools/virtiofsd/passthrough_ll.c
> > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > @@ -2955,7 +2955,8 @@ out:
> > >  }
> > >  
> > >  static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
> > > -                        const char *value, size_t size, int flags)
> > > +                        const char *value, size_t size, int flags,
> > > +                        uint32_t extra_flags)
> > >  {
> > >      char procname[64];
> > >      const char *name;
> > > -- 
> > > 2.25.4
> > > 
> > -- 
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v7 5/7] virtiofsd: Add capability to change/restore umask

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> On Mon, Jun 28, 2021 at 05:12:13PM +0100, Dr. David Alan Gilbert wrote:
> > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > 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 few patches 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 9f5cd98fb5..0c9084ea15 100644
> > > --- a/tools/virtiofsd/passthrough_ll.c
> > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > @@ -122,6 +122,7 @@ struct lo_inode {
> > >  struct lo_cred {
> > >      uid_t euid;
> > >      gid_t egid;
> > > +    mode_t umask;
> > >  };
> > >  
> > >  enum {
> > > @@ -172,6 +173,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[] = {
> > > @@ -1134,7 +1137,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;
> > >  
> > > @@ -1154,11 +1158,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;
> > >  
> > > @@ -1173,6 +1180,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,
> > > @@ -1202,7 +1212,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));
> > 
> > Can you explain what these ISLNK checks are for (insid mknod_symlink, so
> > is that always true or irrelevant?)
> 
> I think I put this check in because if we are creating symlink then we
> don't have to change umask as symlink will always get a some fix
> mode (usually 777) and umask will not have an affect. So this is
> just an optimization to avoid switching umask in some cases. I 
> can't think of any other reason.

But this is in 'lo_mknod_symlink' - so when do we call that except for
making symlinks?

Dave

> thanks
> Vivek
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH v7 5/7] virtiofsd: Add capability to change/restore umask

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> On Mon, Jun 28, 2021 at 05:12:13PM +0100, Dr. David Alan Gilbert wrote:
> > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > 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 few patches 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 9f5cd98fb5..0c9084ea15 100644
> > > --- a/tools/virtiofsd/passthrough_ll.c
> > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > @@ -122,6 +122,7 @@ struct lo_inode {
> > >  struct lo_cred {
> > >      uid_t euid;
> > >      gid_t egid;
> > > +    mode_t umask;
> > >  };
> > >  
> > >  enum {
> > > @@ -172,6 +173,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[] = {
> > > @@ -1134,7 +1137,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;
> > >  
> > > @@ -1154,11 +1158,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;
> > >  
> > > @@ -1173,6 +1180,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,
> > > @@ -1202,7 +1212,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));
> > 
> > Can you explain what these ISLNK checks are for (insid mknod_symlink, so
> > is that always true or irrelevant?)
> 
> I think I put this check in because if we are creating symlink then we
> don't have to change umask as symlink will always get a some fix
> mode (usually 777) and umask will not have an affect. So this is
> just an optimization to avoid switching umask in some cases. I 
> can't think of any other reason.

But this is in 'lo_mknod_symlink' - so when do we call that except for
making symlinks?

Dave

> thanks
> Vivek
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v7 5/7] virtiofsd: Add capability to change/restore umask

[Vivek Goyal]

On Mon, Jun 28, 2021 at 07:36:18PM +0100, Dr. David Alan Gilbert wrote:
> * Vivek Goyal (vgoyal@redhat.com) wrote:
> > On Mon, Jun 28, 2021 at 05:12:13PM +0100, Dr. David Alan Gilbert wrote:
> > > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > > 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 few patches 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 9f5cd98fb5..0c9084ea15 100644
> > > > --- a/tools/virtiofsd/passthrough_ll.c
> > > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > > @@ -122,6 +122,7 @@ struct lo_inode {
> > > >  struct lo_cred {
> > > >      uid_t euid;
> > > >      gid_t egid;
> > > > +    mode_t umask;
> > > >  };
> > > >  
> > > >  enum {
> > > > @@ -172,6 +173,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[] = {
> > > > @@ -1134,7 +1137,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;
> > > >  
> > > > @@ -1154,11 +1158,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;
> > > >  
> > > > @@ -1173,6 +1180,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,
> > > > @@ -1202,7 +1212,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));
> > > 
> > > Can you explain what these ISLNK checks are for (insid mknod_symlink, so
> > > is that always true or irrelevant?)
> > 
> > I think I put this check in because if we are creating symlink then we
> > don't have to change umask as symlink will always get a some fix
> > mode (usually 777) and umask will not have an affect. So this is
> > just an optimization to avoid switching umask in some cases. I 
> > can't think of any other reason.
> 
> But this is in 'lo_mknod_symlink' - so when do we call that except for
> making symlinks?

I think it is called for other mknod paths as well and not limited to
symlink only.


static void lo_mknod(fuse_req_t req, fuse_ino_t parent, const char *name,
                     mode_t mode, dev_t rdev)
{
    lo_mknod_symlink(req, parent, name, mode, rdev, NULL);
}

static void lo_mkdir(fuse_req_t req, fuse_ino_t parent, const char *name,
                     mode_t mode)
{
    lo_mknod_symlink(req, parent, name, S_IFDIR | mode, 0, NULL);
}

static void lo_symlink(fuse_req_t req, const char *link, fuse_ino_t parent,
                       const char *name)
{
    lo_mknod_symlink(req, parent, name, S_IFLNK, 0, link);
}

Vivek

Re: [Virtio-fs] [PATCH v7 5/7] virtiofsd: Add capability to change/restore umask

[Vivek Goyal]

On Mon, Jun 28, 2021 at 07:36:18PM +0100, Dr. David Alan Gilbert wrote:
> * Vivek Goyal (vgoyal@redhat.com) wrote:
> > On Mon, Jun 28, 2021 at 05:12:13PM +0100, Dr. David Alan Gilbert wrote:
> > > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > > 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 few patches 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 9f5cd98fb5..0c9084ea15 100644
> > > > --- a/tools/virtiofsd/passthrough_ll.c
> > > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > > @@ -122,6 +122,7 @@ struct lo_inode {
> > > >  struct lo_cred {
> > > >      uid_t euid;
> > > >      gid_t egid;
> > > > +    mode_t umask;
> > > >  };
> > > >  
> > > >  enum {
> > > > @@ -172,6 +173,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[] = {
> > > > @@ -1134,7 +1137,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;
> > > >  
> > > > @@ -1154,11 +1158,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;
> > > >  
> > > > @@ -1173,6 +1180,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,
> > > > @@ -1202,7 +1212,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));
> > > 
> > > Can you explain what these ISLNK checks are for (insid mknod_symlink, so
> > > is that always true or irrelevant?)
> > 
> > I think I put this check in because if we are creating symlink then we
> > don't have to change umask as symlink will always get a some fix
> > mode (usually 777) and umask will not have an affect. So this is
> > just an optimization to avoid switching umask in some cases. I 
> > can't think of any other reason.
> 
> But this is in 'lo_mknod_symlink' - so when do we call that except for
> making symlinks?

I think it is called for other mknod paths as well and not limited to
symlink only.


static void lo_mknod(fuse_req_t req, fuse_ino_t parent, const char *name,
                     mode_t mode, dev_t rdev)
{
    lo_mknod_symlink(req, parent, name, mode, rdev, NULL);
}

static void lo_mkdir(fuse_req_t req, fuse_ino_t parent, const char *name,
                     mode_t mode)
{
    lo_mknod_symlink(req, parent, name, S_IFDIR | mode, 0, NULL);
}

static void lo_symlink(fuse_req_t req, const char *link, fuse_ino_t parent,
                       const char *name)
{
    lo_mknod_symlink(req, parent, name, S_IFLNK, 0, link);
}

Vivek

Re: [PATCH v7 5/7] virtiofsd: Add capability to change/restore umask

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> On Mon, Jun 28, 2021 at 07:36:18PM +0100, Dr. David Alan Gilbert wrote:
> > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > On Mon, Jun 28, 2021 at 05:12:13PM +0100, Dr. David Alan Gilbert wrote:
> > > > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > > > 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 few patches 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 9f5cd98fb5..0c9084ea15 100644
> > > > > --- a/tools/virtiofsd/passthrough_ll.c
> > > > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > > > @@ -122,6 +122,7 @@ struct lo_inode {
> > > > >  struct lo_cred {
> > > > >      uid_t euid;
> > > > >      gid_t egid;
> > > > > +    mode_t umask;
> > > > >  };
> > > > >  
> > > > >  enum {
> > > > > @@ -172,6 +173,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[] = {
> > > > > @@ -1134,7 +1137,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;
> > > > >  
> > > > > @@ -1154,11 +1158,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;
> > > > >  
> > > > > @@ -1173,6 +1180,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,
> > > > > @@ -1202,7 +1212,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));
> > > > 
> > > > Can you explain what these ISLNK checks are for (insid mknod_symlink, so
> > > > is that always true or irrelevant?)
> > > 
> > > I think I put this check in because if we are creating symlink then we
> > > don't have to change umask as symlink will always get a some fix
> > > mode (usually 777) and umask will not have an affect. So this is
> > > just an optimization to avoid switching umask in some cases. I 
> > > can't think of any other reason.
> > 
> > But this is in 'lo_mknod_symlink' - so when do we call that except for
> > making symlinks?
> 
> I think it is called for other mknod paths as well and not limited to
> symlink only.
> 
> 
> static void lo_mknod(fuse_req_t req, fuse_ino_t parent, const char *name,
>                      mode_t mode, dev_t rdev)
> {
>     lo_mknod_symlink(req, parent, name, mode, rdev, NULL);
> }
> 
> static void lo_mkdir(fuse_req_t req, fuse_ino_t parent, const char *name,
>                      mode_t mode)
> {
>     lo_mknod_symlink(req, parent, name, S_IFDIR | mode, 0, NULL);
> }
> 
> static void lo_symlink(fuse_req_t req, const char *link, fuse_ino_t parent,
>                        const char *name)
> {
>     lo_mknod_symlink(req, parent, name, S_IFLNK, 0, link);
> }

Oh, I see, yeh that confused me - it all then goes through
mknod_wrapper.

Right,

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> Vivek
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH v7 5/7] virtiofsd: Add capability to change/restore umask

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> On Mon, Jun 28, 2021 at 07:36:18PM +0100, Dr. David Alan Gilbert wrote:
> > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > On Mon, Jun 28, 2021 at 05:12:13PM +0100, Dr. David Alan Gilbert wrote:
> > > > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > > > 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 few patches 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 9f5cd98fb5..0c9084ea15 100644
> > > > > --- a/tools/virtiofsd/passthrough_ll.c
> > > > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > > > @@ -122,6 +122,7 @@ struct lo_inode {
> > > > >  struct lo_cred {
> > > > >      uid_t euid;
> > > > >      gid_t egid;
> > > > > +    mode_t umask;
> > > > >  };
> > > > >  
> > > > >  enum {
> > > > > @@ -172,6 +173,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[] = {
> > > > > @@ -1134,7 +1137,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;
> > > > >  
> > > > > @@ -1154,11 +1158,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;
> > > > >  
> > > > > @@ -1173,6 +1180,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,
> > > > > @@ -1202,7 +1212,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));
> > > > 
> > > > Can you explain what these ISLNK checks are for (insid mknod_symlink, so
> > > > is that always true or irrelevant?)
> > > 
> > > I think I put this check in because if we are creating symlink then we
> > > don't have to change umask as symlink will always get a some fix
> > > mode (usually 777) and umask will not have an affect. So this is
> > > just an optimization to avoid switching umask in some cases. I 
> > > can't think of any other reason.
> > 
> > But this is in 'lo_mknod_symlink' - so when do we call that except for
> > making symlinks?
> 
> I think it is called for other mknod paths as well and not limited to
> symlink only.
> 
> 
> static void lo_mknod(fuse_req_t req, fuse_ino_t parent, const char *name,
>                      mode_t mode, dev_t rdev)
> {
>     lo_mknod_symlink(req, parent, name, mode, rdev, NULL);
> }
> 
> static void lo_mkdir(fuse_req_t req, fuse_ino_t parent, const char *name,
>                      mode_t mode)
> {
>     lo_mknod_symlink(req, parent, name, S_IFDIR | mode, 0, NULL);
> }
> 
> static void lo_symlink(fuse_req_t req, const char *link, fuse_ino_t parent,
>                        const char *name)
> {
>     lo_mknod_symlink(req, parent, name, S_IFLNK, 0, link);
> }

Oh, I see, yeh that confused me - it all then goes through
mknod_wrapper.

Right,

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> Vivek
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH v7 1/7] virtiofsd: Fix fuse setxattr() API change issue

[Greg Kurz]

On Mon, 28 Jun 2021 15:46:40 +0100
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> * Vivek Goyal (vgoyal@redhat.com) wrote:
> > With kernel header updates fuse_setxattr_in struct has grown in size.
> > But this new struct size only takes affect if user has opted in
> > for fuse feature FUSE_SETXATTR_EXT otherwise fuse continues to
> > send "fuse_setxattr_in" of older size. Older size is determined
> > by FUSE_COMPAT_SETXATTR_IN_SIZE.
> > 
> > Fix this. If we have not opted in for FUSE_SETXATTR_EXT, then
> > expect that we will get fuse_setxattr_in of size FUSE_COMPAT_SETXATTR_IN_SIZE
> > and not sizeof(struct fuse_sexattr_in).
> > 
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> 
> Yeh it's a bit of a grim fix, but I think it's the best we can do, and
> we need to get it in since the headers have already been merged.

You can also add:

Fixes: 278f064e4524 ("Update Linux headers to 5.13-rc4")

because this is basically what happened : this commit exposes the API
breakage.

This is kinda problematic : linux kernel headers are updated globally,
i.e. an header update merged by some other subsystem will unknowingly
break virtiofsd each time we face a similar situation.

We could cope with it if the code was adapted to API changes when
needed, e.g. this patch squashed into 278f064e4524 . It doesn't
seem that can be achievable without some automation to detect
buggy situations (e.g. code depends on the size of a structure).
And even with that, it would still cause the subsystem that
needs the header update to depend on other subsystems to
fix the breakage.

Another possibility could maybe to stop doing global updates and
let each subsystem handle the kernel headers it needs.

OR we could avoid breaking the API in the kernel in the first
place.

Thoughts ?

Anyway, the fix is good:

Reviewed-by: Greg Kurz <groug@kaod.org>

> (I don't think libfuse has a fix for this in yet itself, but it might
> survive because it doesn't bother copying the data like we do with
> mbuf).
> 
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> > ---
> >  tools/virtiofsd/fuse_common.h   | 5 +++++
> >  tools/virtiofsd/fuse_lowlevel.c | 7 ++++++-
> >  2 files changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/virtiofsd/fuse_common.h b/tools/virtiofsd/fuse_common.h
> > index fa9671872e..0c2665b977 100644
> > --- a/tools/virtiofsd/fuse_common.h
> > +++ b/tools/virtiofsd/fuse_common.h
> > @@ -372,6 +372,11 @@ struct fuse_file_info {
> >   */
> >  #define FUSE_CAP_HANDLE_KILLPRIV_V2 (1 << 28)
> >  
> > +/**
> > + * Indicates that file server supports extended struct fuse_setxattr_in
> > + */
> > +#define FUSE_CAP_SETXATTR_EXT (1 << 29)
> > +
> >  /**
> >   * Ioctl flags
> >   *
> > diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
> > index 7fe2cef1eb..c2b6ff1686 100644
> > --- a/tools/virtiofsd/fuse_lowlevel.c
> > +++ b/tools/virtiofsd/fuse_lowlevel.c
> > @@ -1419,8 +1419,13 @@ static void do_setxattr(fuse_req_t req, fuse_ino_t nodeid,
> >      struct fuse_setxattr_in *arg;
> >      const char *name;
> >      const char *value;
> > +    bool setxattr_ext = req->se->conn.want & FUSE_CAP_SETXATTR_EXT;
> >  
> > -    arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
> > +    if (setxattr_ext) {
> > +        arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
> > +    } else {
> > +        arg = fuse_mbuf_iter_advance(iter, FUSE_COMPAT_SETXATTR_IN_SIZE);
> > +    }
> >      name = fuse_mbuf_iter_advance_str(iter);
> >      if (!arg || !name) {
> >          fuse_reply_err(req, EINVAL);
> > -- 
> > 2.25.4
> > 

Re: [Virtio-fs] [PATCH v7 2/7] virtiofsd: Fix xattr operations overwriting errno

[Greg Kurz]

On Mon, 28 Jun 2021 16:31:27 +0100
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> * Vivek Goyal (vgoyal@redhat.com) wrote:
> > getxattr/setxattr/removexattr/listxattr operations handle regualar
> > and non-regular files differently. For the case of non-regular files
> > we do fchdir(/proc/self/fd) and the xattr operation and then revert
> > back to original working directory. After this we are saving errno
> > and that's buggy because fchdir() will overwrite the errno.
> > 
> > FCHDIR_NOFAIL(lo->proc_self_fd);
> > ret = getxattr(procname, name, value, size);
> > FCHDIR_NOFAIL(lo->root.fd);
> > 
> > if (ret == -1)
> >     saverr = errno
> > 
> > In above example, if getxattr() failed, we will still return 0 to caller
> > as errno must have been written by FCHDIR_NOFAIL(lo->root.fd) call.

This assertion doesn't look right.

From the errno(3) manual page:

       The value in errno is significant only when the return value of
       the call indicated an error (i.e., -1 from most system calls; -1
       or NULL from most library functions); a function that succeeds is
       allowed to change errno.  The value of errno is never set to zero
                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^
       by any system call or library function.

> > Fix all such instances and capture "errno" early and save in "saverr"
> > variable.
> > 
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> 
> I think the existing code is actually safe; I don't think fchdir
> will/should set errno unless there's an error, and we're explictly
> asserting there isn't one.
> 
> However, I do prefer doing this save since we already have the save
> variables and it makes the chance of screwing it up from any other
> forgotten syscall less likely, so
> 

Agreed. With this rationale in the changelog:

Reviewed-by: Greg Kurz <groug@kaod.org>

> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> > ---
> >  tools/virtiofsd/passthrough_ll.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > index 49c21fd855..ec91b3c133 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -2791,15 +2791,17 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
> >              goto out_err;
> >          }
> >          ret = fgetxattr(fd, name, value, size);
> > +        saverr = ret == -1 ? errno : 0;
> >      } else {
> >          /* fchdir should not fail here */
> >          FCHDIR_NOFAIL(lo->proc_self_fd);
> >          ret = getxattr(procname, name, value, size);
> > +        saverr = ret == -1 ? errno : 0;
> >          FCHDIR_NOFAIL(lo->root.fd);
> >      }
> >  
> >      if (ret == -1) {
> > -        goto out_err;
> > +        goto out;
> >      }
> >      if (size) {
> >          saverr = 0;
> > @@ -2864,15 +2866,17 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
> >              goto out_err;
> >          }
> >          ret = flistxattr(fd, value, size);
> > +        saverr = ret == -1 ? errno : 0;
> >      } else {
> >          /* fchdir should not fail here */
> >          FCHDIR_NOFAIL(lo->proc_self_fd);
> >          ret = listxattr(procname, value, size);
> > +        saverr = ret == -1 ? errno : 0;
> >          FCHDIR_NOFAIL(lo->root.fd);
> >      }
> >  
> >      if (ret == -1) {
> > -        goto out_err;
> > +        goto out;
> >      }
> >      if (size) {
> >          saverr = 0;
> > @@ -2998,15 +3002,15 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
> >              goto out;
> >          }
> >          ret = fsetxattr(fd, name, value, size, flags);
> > +        saverr = ret == -1 ? errno : 0;
> >      } else {
> >          /* fchdir should not fail here */
> >          FCHDIR_NOFAIL(lo->proc_self_fd);
> >          ret = setxattr(procname, name, value, size, flags);
> > +        saverr = ret == -1 ? errno : 0;
> >          FCHDIR_NOFAIL(lo->root.fd);
> >      }
> >  
> > -    saverr = ret == -1 ? errno : 0;
> > -
> >  out:
> >      if (fd >= 0) {
> >          close(fd);
> > @@ -3064,15 +3068,15 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *in_name)
> >              goto out;
> >          }
> >          ret = fremovexattr(fd, name);
> > +        saverr = ret == -1 ? errno : 0;
> >      } else {
> >          /* fchdir should not fail here */
> >          FCHDIR_NOFAIL(lo->proc_self_fd);
> >          ret = removexattr(procname, name);
> > +        saverr = ret == -1 ? errno : 0;
> >          FCHDIR_NOFAIL(lo->root.fd);
> >      }
> >  
> > -    saverr = ret == -1 ? errno : 0;
> > -
> >  out:
> >      if (fd >= 0) {
> >          close(fd);
> > -- 
> > 2.25.4
> > 

Re: [Virtio-fs] [PATCH v7 2/7] virtiofsd: Fix xattr operations overwriting errno

[Greg Kurz]

On Mon, 28 Jun 2021 16:31:27 +0100
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> * Vivek Goyal (vgoyal@redhat.com) wrote:
> > getxattr/setxattr/removexattr/listxattr operations handle regualar
> > and non-regular files differently. For the case of non-regular files
> > we do fchdir(/proc/self/fd) and the xattr operation and then revert
> > back to original working directory. After this we are saving errno
> > and that's buggy because fchdir() will overwrite the errno.
> > 
> > FCHDIR_NOFAIL(lo->proc_self_fd);
> > ret = getxattr(procname, name, value, size);
> > FCHDIR_NOFAIL(lo->root.fd);
> > 
> > if (ret == -1)
> >     saverr = errno
> > 
> > In above example, if getxattr() failed, we will still return 0 to caller
> > as errno must have been written by FCHDIR_NOFAIL(lo->root.fd) call.

This assertion doesn't look right.

>From the errno(3) manual page:

       The value in errno is significant only when the return value of
       the call indicated an error (i.e., -1 from most system calls; -1
       or NULL from most library functions); a function that succeeds is
       allowed to change errno.  The value of errno is never set to zero
                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^
       by any system call or library function.

> > Fix all such instances and capture "errno" early and save in "saverr"
> > variable.
> > 
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> 
> I think the existing code is actually safe; I don't think fchdir
> will/should set errno unless there's an error, and we're explictly
> asserting there isn't one.
> 
> However, I do prefer doing this save since we already have the save
> variables and it makes the chance of screwing it up from any other
> forgotten syscall less likely, so
> 

Agreed. With this rationale in the changelog:

Reviewed-by: Greg Kurz <groug@kaod.org>

> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> > ---
> >  tools/virtiofsd/passthrough_ll.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > index 49c21fd855..ec91b3c133 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -2791,15 +2791,17 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
> >              goto out_err;
> >          }
> >          ret = fgetxattr(fd, name, value, size);
> > +        saverr = ret == -1 ? errno : 0;
> >      } else {
> >          /* fchdir should not fail here */
> >          FCHDIR_NOFAIL(lo->proc_self_fd);
> >          ret = getxattr(procname, name, value, size);
> > +        saverr = ret == -1 ? errno : 0;
> >          FCHDIR_NOFAIL(lo->root.fd);
> >      }
> >  
> >      if (ret == -1) {
> > -        goto out_err;
> > +        goto out;
> >      }
> >      if (size) {
> >          saverr = 0;
> > @@ -2864,15 +2866,17 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
> >              goto out_err;
> >          }
> >          ret = flistxattr(fd, value, size);
> > +        saverr = ret == -1 ? errno : 0;
> >      } else {
> >          /* fchdir should not fail here */
> >          FCHDIR_NOFAIL(lo->proc_self_fd);
> >          ret = listxattr(procname, value, size);
> > +        saverr = ret == -1 ? errno : 0;
> >          FCHDIR_NOFAIL(lo->root.fd);
> >      }
> >  
> >      if (ret == -1) {
> > -        goto out_err;
> > +        goto out;
> >      }
> >      if (size) {
> >          saverr = 0;
> > @@ -2998,15 +3002,15 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
> >              goto out;
> >          }
> >          ret = fsetxattr(fd, name, value, size, flags);
> > +        saverr = ret == -1 ? errno : 0;
> >      } else {
> >          /* fchdir should not fail here */
> >          FCHDIR_NOFAIL(lo->proc_self_fd);
> >          ret = setxattr(procname, name, value, size, flags);
> > +        saverr = ret == -1 ? errno : 0;
> >          FCHDIR_NOFAIL(lo->root.fd);
> >      }
> >  
> > -    saverr = ret == -1 ? errno : 0;
> > -
> >  out:
> >      if (fd >= 0) {
> >          close(fd);
> > @@ -3064,15 +3068,15 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *in_name)
> >              goto out;
> >          }
> >          ret = fremovexattr(fd, name);
> > +        saverr = ret == -1 ? errno : 0;
> >      } else {
> >          /* fchdir should not fail here */
> >          FCHDIR_NOFAIL(lo->proc_self_fd);
> >          ret = removexattr(procname, name);
> > +        saverr = ret == -1 ? errno : 0;
> >          FCHDIR_NOFAIL(lo->root.fd);
> >      }
> >  
> > -    saverr = ret == -1 ? errno : 0;
> > -
> >  out:
> >      if (fd >= 0) {
> >          close(fd);
> > -- 
> > 2.25.4
> > 

Re: [Virtio-fs] [PATCH v7 2/7] virtiofsd: Fix xattr operations overwriting errno

[Vivek Goyal]

On Tue, Jun 29, 2021 at 03:03:43PM +0200, Greg Kurz wrote:
> On Mon, 28 Jun 2021 16:31:27 +0100
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> 
> > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > getxattr/setxattr/removexattr/listxattr operations handle regualar
> > > and non-regular files differently. For the case of non-regular files
> > > we do fchdir(/proc/self/fd) and the xattr operation and then revert
> > > back to original working directory. After this we are saving errno
> > > and that's buggy because fchdir() will overwrite the errno.
> > > 
> > > FCHDIR_NOFAIL(lo->proc_self_fd);
> > > ret = getxattr(procname, name, value, size);
> > > FCHDIR_NOFAIL(lo->root.fd);
> > > 
> > > if (ret == -1)
> > >     saverr = errno
> > > 
> > > In above example, if getxattr() failed, we will still return 0 to caller
> > > as errno must have been written by FCHDIR_NOFAIL(lo->root.fd) call.
> 
> This assertion doesn't look right.
> 
> From the errno(3) manual page:
> 
>        The value in errno is significant only when the return value of
>        the call indicated an error (i.e., -1 from most system calls; -1
>        or NULL from most library functions); a function that succeeds is
>        allowed to change errno.  The value of errno is never set to zero
>                                               ^^^^^^^^^^^^^^^^^^^^^^^^^^
>        by any system call or library function.

Ok. So it will not set errno to 0. But it also says "a function that
succeeds is allowed to change errno". So that means a successful
fchdir(fd) can change errno to something else and we lost original
error code returned by getxattr() operation. Even "assert(fchdir_res == 0)"
will not kick in because fchdir() succeeded.

IOW, with current code we can still lose original error code as experienced
while executing getxattr()/setxattr()/listxattr(). So it makese sense
to fix it.

Vivek

> > > Fix all such instances and capture "errno" early and save in "saverr"
> > > variable.
> > > 
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > 
> > I think the existing code is actually safe; I don't think fchdir
> > will/should set errno unless there's an error, and we're explictly
> > asserting there isn't one.
> > 
> > However, I do prefer doing this save since we already have the save
> > variables and it makes the chance of screwing it up from any other
> > forgotten syscall less likely, so
> > 
> 
> Agreed. With this rationale in the changelog:
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> > 
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > 
> > > ---
> > >  tools/virtiofsd/passthrough_ll.c | 16 ++++++++++------
> > >  1 file changed, 10 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > index 49c21fd855..ec91b3c133 100644
> > > --- a/tools/virtiofsd/passthrough_ll.c
> > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > @@ -2791,15 +2791,17 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
> > >              goto out_err;
> > >          }
> > >          ret = fgetxattr(fd, name, value, size);
> > > +        saverr = ret == -1 ? errno : 0;
> > >      } else {
> > >          /* fchdir should not fail here */
> > >          FCHDIR_NOFAIL(lo->proc_self_fd);
> > >          ret = getxattr(procname, name, value, size);
> > > +        saverr = ret == -1 ? errno : 0;
> > >          FCHDIR_NOFAIL(lo->root.fd);
> > >      }
> > >  
> > >      if (ret == -1) {
> > > -        goto out_err;
> > > +        goto out;
> > >      }
> > >      if (size) {
> > >          saverr = 0;
> > > @@ -2864,15 +2866,17 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
> > >              goto out_err;
> > >          }
> > >          ret = flistxattr(fd, value, size);
> > > +        saverr = ret == -1 ? errno : 0;
> > >      } else {
> > >          /* fchdir should not fail here */
> > >          FCHDIR_NOFAIL(lo->proc_self_fd);
> > >          ret = listxattr(procname, value, size);
> > > +        saverr = ret == -1 ? errno : 0;
> > >          FCHDIR_NOFAIL(lo->root.fd);
> > >      }
> > >  
> > >      if (ret == -1) {
> > > -        goto out_err;
> > > +        goto out;
> > >      }
> > >      if (size) {
> > >          saverr = 0;
> > > @@ -2998,15 +3002,15 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
> > >              goto out;
> > >          }
> > >          ret = fsetxattr(fd, name, value, size, flags);
> > > +        saverr = ret == -1 ? errno : 0;
> > >      } else {
> > >          /* fchdir should not fail here */
> > >          FCHDIR_NOFAIL(lo->proc_self_fd);
> > >          ret = setxattr(procname, name, value, size, flags);
> > > +        saverr = ret == -1 ? errno : 0;
> > >          FCHDIR_NOFAIL(lo->root.fd);
> > >      }
> > >  
> > > -    saverr = ret == -1 ? errno : 0;
> > > -
> > >  out:
> > >      if (fd >= 0) {
> > >          close(fd);
> > > @@ -3064,15 +3068,15 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *in_name)
> > >              goto out;
> > >          }
> > >          ret = fremovexattr(fd, name);
> > > +        saverr = ret == -1 ? errno : 0;
> > >      } else {
> > >          /* fchdir should not fail here */
> > >          FCHDIR_NOFAIL(lo->proc_self_fd);
> > >          ret = removexattr(procname, name);
> > > +        saverr = ret == -1 ? errno : 0;
> > >          FCHDIR_NOFAIL(lo->root.fd);
> > >      }
> > >  
> > > -    saverr = ret == -1 ? errno : 0;
> > > -
> > >  out:
> > >      if (fd >= 0) {
> > >          close(fd);
> > > -- 
> > > 2.25.4
> > > 
> 

Re: [Virtio-fs] [PATCH v7 2/7] virtiofsd: Fix xattr operations overwriting errno

[Vivek Goyal]

On Tue, Jun 29, 2021 at 03:03:43PM +0200, Greg Kurz wrote:
> On Mon, 28 Jun 2021 16:31:27 +0100
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> 
> > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > getxattr/setxattr/removexattr/listxattr operations handle regualar
> > > and non-regular files differently. For the case of non-regular files
> > > we do fchdir(/proc/self/fd) and the xattr operation and then revert
> > > back to original working directory. After this we are saving errno
> > > and that's buggy because fchdir() will overwrite the errno.
> > > 
> > > FCHDIR_NOFAIL(lo->proc_self_fd);
> > > ret = getxattr(procname, name, value, size);
> > > FCHDIR_NOFAIL(lo->root.fd);
> > > 
> > > if (ret == -1)
> > >     saverr = errno
> > > 
> > > In above example, if getxattr() failed, we will still return 0 to caller
> > > as errno must have been written by FCHDIR_NOFAIL(lo->root.fd) call.
> 
> This assertion doesn't look right.
> 
> From the errno(3) manual page:
> 
>        The value in errno is significant only when the return value of
>        the call indicated an error (i.e., -1 from most system calls; -1
>        or NULL from most library functions); a function that succeeds is
>        allowed to change errno.  The value of errno is never set to zero
>                                               ^^^^^^^^^^^^^^^^^^^^^^^^^^
>        by any system call or library function.

Ok. So it will not set errno to 0. But it also says "a function that
succeeds is allowed to change errno". So that means a successful
fchdir(fd) can change errno to something else and we lost original
error code returned by getxattr() operation. Even "assert(fchdir_res == 0)"
will not kick in because fchdir() succeeded.

IOW, with current code we can still lose original error code as experienced
while executing getxattr()/setxattr()/listxattr(). So it makese sense
to fix it.

Vivek

> > > Fix all such instances and capture "errno" early and save in "saverr"
> > > variable.
> > > 
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > 
> > I think the existing code is actually safe; I don't think fchdir
> > will/should set errno unless there's an error, and we're explictly
> > asserting there isn't one.
> > 
> > However, I do prefer doing this save since we already have the save
> > variables and it makes the chance of screwing it up from any other
> > forgotten syscall less likely, so
> > 
> 
> Agreed. With this rationale in the changelog:
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> > 
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > 
> > > ---
> > >  tools/virtiofsd/passthrough_ll.c | 16 ++++++++++------
> > >  1 file changed, 10 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > index 49c21fd855..ec91b3c133 100644
> > > --- a/tools/virtiofsd/passthrough_ll.c
> > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > @@ -2791,15 +2791,17 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
> > >              goto out_err;
> > >          }
> > >          ret = fgetxattr(fd, name, value, size);
> > > +        saverr = ret == -1 ? errno : 0;
> > >      } else {
> > >          /* fchdir should not fail here */
> > >          FCHDIR_NOFAIL(lo->proc_self_fd);
> > >          ret = getxattr(procname, name, value, size);
> > > +        saverr = ret == -1 ? errno : 0;
> > >          FCHDIR_NOFAIL(lo->root.fd);
> > >      }
> > >  
> > >      if (ret == -1) {
> > > -        goto out_err;
> > > +        goto out;
> > >      }
> > >      if (size) {
> > >          saverr = 0;
> > > @@ -2864,15 +2866,17 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
> > >              goto out_err;
> > >          }
> > >          ret = flistxattr(fd, value, size);
> > > +        saverr = ret == -1 ? errno : 0;
> > >      } else {
> > >          /* fchdir should not fail here */
> > >          FCHDIR_NOFAIL(lo->proc_self_fd);
> > >          ret = listxattr(procname, value, size);
> > > +        saverr = ret == -1 ? errno : 0;
> > >          FCHDIR_NOFAIL(lo->root.fd);
> > >      }
> > >  
> > >      if (ret == -1) {
> > > -        goto out_err;
> > > +        goto out;
> > >      }
> > >      if (size) {
> > >          saverr = 0;
> > > @@ -2998,15 +3002,15 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
> > >              goto out;
> > >          }
> > >          ret = fsetxattr(fd, name, value, size, flags);
> > > +        saverr = ret == -1 ? errno : 0;
> > >      } else {
> > >          /* fchdir should not fail here */
> > >          FCHDIR_NOFAIL(lo->proc_self_fd);
> > >          ret = setxattr(procname, name, value, size, flags);
> > > +        saverr = ret == -1 ? errno : 0;
> > >          FCHDIR_NOFAIL(lo->root.fd);
> > >      }
> > >  
> > > -    saverr = ret == -1 ? errno : 0;
> > > -
> > >  out:
> > >      if (fd >= 0) {
> > >          close(fd);
> > > @@ -3064,15 +3068,15 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *in_name)
> > >              goto out;
> > >          }
> > >          ret = fremovexattr(fd, name);
> > > +        saverr = ret == -1 ? errno : 0;
> > >      } else {
> > >          /* fchdir should not fail here */
> > >          FCHDIR_NOFAIL(lo->proc_self_fd);
> > >          ret = removexattr(procname, name);
> > > +        saverr = ret == -1 ? errno : 0;
> > >          FCHDIR_NOFAIL(lo->root.fd);
> > >      }
> > >  
> > > -    saverr = ret == -1 ? errno : 0;
> > > -
> > >  out:
> > >      if (fd >= 0) {
> > >          close(fd);
> > > -- 
> > > 2.25.4
> > > 
> 

Re: [Virtio-fs] [PATCH v7 2/7] virtiofsd: Fix xattr operations overwriting errno

[Greg Kurz]

On Tue, 29 Jun 2021 09:22:36 -0400
Vivek Goyal <vgoyal@redhat.com> wrote:

> On Tue, Jun 29, 2021 at 03:03:43PM +0200, Greg Kurz wrote:
> > On Mon, 28 Jun 2021 16:31:27 +0100
> > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > 
> > > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > > getxattr/setxattr/removexattr/listxattr operations handle regualar
> > > > and non-regular files differently. For the case of non-regular files
> > > > we do fchdir(/proc/self/fd) and the xattr operation and then revert
> > > > back to original working directory. After this we are saving errno
> > > > and that's buggy because fchdir() will overwrite the errno.
> > > > 
> > > > FCHDIR_NOFAIL(lo->proc_self_fd);
> > > > ret = getxattr(procname, name, value, size);
> > > > FCHDIR_NOFAIL(lo->root.fd);
> > > > 
> > > > if (ret == -1)
> > > >     saverr = errno
> > > > 
> > > > In above example, if getxattr() failed, we will still return 0 to caller
> > > > as errno must have been written by FCHDIR_NOFAIL(lo->root.fd) call.
> > 
> > This assertion doesn't look right.
> > 
> > From the errno(3) manual page:
> > 
> >        The value in errno is significant only when the return value of
> >        the call indicated an error (i.e., -1 from most system calls; -1
> >        or NULL from most library functions); a function that succeeds is
> >        allowed to change errno.  The value of errno is never set to zero
> >                                               ^^^^^^^^^^^^^^^^^^^^^^^^^^
> >        by any system call or library function.
> 
> Ok. So it will not set errno to 0. But it also says "a function that
> succeeds is allowed to change errno". So that means a successful
> fchdir(fd) can change errno to something else and we lost original
> error code returned by getxattr() operation. Even "assert(fchdir_res == 0)"
> will not kick in because fchdir() succeeded.
> 
> IOW, with current code we can still lose original error code as experienced
> while executing getxattr()/setxattr()/listxattr(). So it makese sense
> to fix it.
> 

Sure ! I wasn't challenging the fix, but rather the "still return 0 to caller"
wording :)

Cheers,

--
Greg

> Vivek
> 
> > > > Fix all such instances and capture "errno" early and save in "saverr"
> > > > variable.
> > > > 
> > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > 
> > > I think the existing code is actually safe; I don't think fchdir
> > > will/should set errno unless there's an error, and we're explictly
> > > asserting there isn't one.
> > > 
> > > However, I do prefer doing this save since we already have the save
> > > variables and it makes the chance of screwing it up from any other
> > > forgotten syscall less likely, so
> > > 
> > 
> > Agreed. With this rationale in the changelog:
> > 
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> > 
> > > 
> > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > 
> > > > ---
> > > >  tools/virtiofsd/passthrough_ll.c | 16 ++++++++++------
> > > >  1 file changed, 10 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > > index 49c21fd855..ec91b3c133 100644
> > > > --- a/tools/virtiofsd/passthrough_ll.c
> > > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > > @@ -2791,15 +2791,17 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
> > > >              goto out_err;
> > > >          }
> > > >          ret = fgetxattr(fd, name, value, size);
> > > > +        saverr = ret == -1 ? errno : 0;
> > > >      } else {
> > > >          /* fchdir should not fail here */
> > > >          FCHDIR_NOFAIL(lo->proc_self_fd);
> > > >          ret = getxattr(procname, name, value, size);
> > > > +        saverr = ret == -1 ? errno : 0;
> > > >          FCHDIR_NOFAIL(lo->root.fd);
> > > >      }
> > > >  
> > > >      if (ret == -1) {
> > > > -        goto out_err;
> > > > +        goto out;
> > > >      }
> > > >      if (size) {
> > > >          saverr = 0;
> > > > @@ -2864,15 +2866,17 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
> > > >              goto out_err;
> > > >          }
> > > >          ret = flistxattr(fd, value, size);
> > > > +        saverr = ret == -1 ? errno : 0;
> > > >      } else {
> > > >          /* fchdir should not fail here */
> > > >          FCHDIR_NOFAIL(lo->proc_self_fd);
> > > >          ret = listxattr(procname, value, size);
> > > > +        saverr = ret == -1 ? errno : 0;
> > > >          FCHDIR_NOFAIL(lo->root.fd);
> > > >      }
> > > >  
> > > >      if (ret == -1) {
> > > > -        goto out_err;
> > > > +        goto out;
> > > >      }
> > > >      if (size) {
> > > >          saverr = 0;
> > > > @@ -2998,15 +3002,15 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
> > > >              goto out;
> > > >          }
> > > >          ret = fsetxattr(fd, name, value, size, flags);
> > > > +        saverr = ret == -1 ? errno : 0;
> > > >      } else {
> > > >          /* fchdir should not fail here */
> > > >          FCHDIR_NOFAIL(lo->proc_self_fd);
> > > >          ret = setxattr(procname, name, value, size, flags);
> > > > +        saverr = ret == -1 ? errno : 0;
> > > >          FCHDIR_NOFAIL(lo->root.fd);
> > > >      }
> > > >  
> > > > -    saverr = ret == -1 ? errno : 0;
> > > > -
> > > >  out:
> > > >      if (fd >= 0) {
> > > >          close(fd);
> > > > @@ -3064,15 +3068,15 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *in_name)
> > > >              goto out;
> > > >          }
> > > >          ret = fremovexattr(fd, name);
> > > > +        saverr = ret == -1 ? errno : 0;
> > > >      } else {
> > > >          /* fchdir should not fail here */
> > > >          FCHDIR_NOFAIL(lo->proc_self_fd);
> > > >          ret = removexattr(procname, name);
> > > > +        saverr = ret == -1 ? errno : 0;
> > > >          FCHDIR_NOFAIL(lo->root.fd);
> > > >      }
> > > >  
> > > > -    saverr = ret == -1 ? errno : 0;
> > > > -
> > > >  out:
> > > >      if (fd >= 0) {
> > > >          close(fd);
> > > > -- 
> > > > 2.25.4
> > > > 
> > 
> 

Re: [Virtio-fs] [PATCH v7 2/7] virtiofsd: Fix xattr operations overwriting errno

[Greg Kurz]

On Tue, 29 Jun 2021 09:22:36 -0400
Vivek Goyal <vgoyal@redhat.com> wrote:

> On Tue, Jun 29, 2021 at 03:03:43PM +0200, Greg Kurz wrote:
> > On Mon, 28 Jun 2021 16:31:27 +0100
> > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > 
> > > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > > getxattr/setxattr/removexattr/listxattr operations handle regualar
> > > > and non-regular files differently. For the case of non-regular files
> > > > we do fchdir(/proc/self/fd) and the xattr operation and then revert
> > > > back to original working directory. After this we are saving errno
> > > > and that's buggy because fchdir() will overwrite the errno.
> > > > 
> > > > FCHDIR_NOFAIL(lo->proc_self_fd);
> > > > ret = getxattr(procname, name, value, size);
> > > > FCHDIR_NOFAIL(lo->root.fd);
> > > > 
> > > > if (ret == -1)
> > > >     saverr = errno
> > > > 
> > > > In above example, if getxattr() failed, we will still return 0 to caller
> > > > as errno must have been written by FCHDIR_NOFAIL(lo->root.fd) call.
> > 
> > This assertion doesn't look right.
> > 
> > From the errno(3) manual page:
> > 
> >        The value in errno is significant only when the return value of
> >        the call indicated an error (i.e., -1 from most system calls; -1
> >        or NULL from most library functions); a function that succeeds is
> >        allowed to change errno.  The value of errno is never set to zero
> >                                               ^^^^^^^^^^^^^^^^^^^^^^^^^^
> >        by any system call or library function.
> 
> Ok. So it will not set errno to 0. But it also says "a function that
> succeeds is allowed to change errno". So that means a successful
> fchdir(fd) can change errno to something else and we lost original
> error code returned by getxattr() operation. Even "assert(fchdir_res == 0)"
> will not kick in because fchdir() succeeded.
> 
> IOW, with current code we can still lose original error code as experienced
> while executing getxattr()/setxattr()/listxattr(). So it makese sense
> to fix it.
> 

Sure ! I wasn't challenging the fix, but rather the "still return 0 to caller"
wording :)

Cheers,

--
Greg

> Vivek
> 
> > > > Fix all such instances and capture "errno" early and save in "saverr"
> > > > variable.
> > > > 
> > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > 
> > > I think the existing code is actually safe; I don't think fchdir
> > > will/should set errno unless there's an error, and we're explictly
> > > asserting there isn't one.
> > > 
> > > However, I do prefer doing this save since we already have the save
> > > variables and it makes the chance of screwing it up from any other
> > > forgotten syscall less likely, so
> > > 
> > 
> > Agreed. With this rationale in the changelog:
> > 
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> > 
> > > 
> > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > 
> > > > ---
> > > >  tools/virtiofsd/passthrough_ll.c | 16 ++++++++++------
> > > >  1 file changed, 10 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > > index 49c21fd855..ec91b3c133 100644
> > > > --- a/tools/virtiofsd/passthrough_ll.c
> > > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > > @@ -2791,15 +2791,17 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
> > > >              goto out_err;
> > > >          }
> > > >          ret = fgetxattr(fd, name, value, size);
> > > > +        saverr = ret == -1 ? errno : 0;
> > > >      } else {
> > > >          /* fchdir should not fail here */
> > > >          FCHDIR_NOFAIL(lo->proc_self_fd);
> > > >          ret = getxattr(procname, name, value, size);
> > > > +        saverr = ret == -1 ? errno : 0;
> > > >          FCHDIR_NOFAIL(lo->root.fd);
> > > >      }
> > > >  
> > > >      if (ret == -1) {
> > > > -        goto out_err;
> > > > +        goto out;
> > > >      }
> > > >      if (size) {
> > > >          saverr = 0;
> > > > @@ -2864,15 +2866,17 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
> > > >              goto out_err;
> > > >          }
> > > >          ret = flistxattr(fd, value, size);
> > > > +        saverr = ret == -1 ? errno : 0;
> > > >      } else {
> > > >          /* fchdir should not fail here */
> > > >          FCHDIR_NOFAIL(lo->proc_self_fd);
> > > >          ret = listxattr(procname, value, size);
> > > > +        saverr = ret == -1 ? errno : 0;
> > > >          FCHDIR_NOFAIL(lo->root.fd);
> > > >      }
> > > >  
> > > >      if (ret == -1) {
> > > > -        goto out_err;
> > > > +        goto out;
> > > >      }
> > > >      if (size) {
> > > >          saverr = 0;
> > > > @@ -2998,15 +3002,15 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
> > > >              goto out;
> > > >          }
> > > >          ret = fsetxattr(fd, name, value, size, flags);
> > > > +        saverr = ret == -1 ? errno : 0;
> > > >      } else {
> > > >          /* fchdir should not fail here */
> > > >          FCHDIR_NOFAIL(lo->proc_self_fd);
> > > >          ret = setxattr(procname, name, value, size, flags);
> > > > +        saverr = ret == -1 ? errno : 0;
> > > >          FCHDIR_NOFAIL(lo->root.fd);
> > > >      }
> > > >  
> > > > -    saverr = ret == -1 ? errno : 0;
> > > > -
> > > >  out:
> > > >      if (fd >= 0) {
> > > >          close(fd);
> > > > @@ -3064,15 +3068,15 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *in_name)
> > > >              goto out;
> > > >          }
> > > >          ret = fremovexattr(fd, name);
> > > > +        saverr = ret == -1 ? errno : 0;
> > > >      } else {
> > > >          /* fchdir should not fail here */
> > > >          FCHDIR_NOFAIL(lo->proc_self_fd);
> > > >          ret = removexattr(procname, name);
> > > > +        saverr = ret == -1 ? errno : 0;
> > > >          FCHDIR_NOFAIL(lo->root.fd);
> > > >      }
> > > >  
> > > > -    saverr = ret == -1 ? errno : 0;
> > > > -
> > > >  out:
> > > >      if (fd >= 0) {
> > > >          close(fd);
> > > > -- 
> > > > 2.25.4
> > > > 
> > 
> 

Re: [Virtio-fs] [PATCH v7 1/7] virtiofsd: Fix fuse setxattr() API change issue

[Dr. David Alan Gilbert]

* Greg Kurz (groug@kaod.org) wrote:
> On Mon, 28 Jun 2021 15:46:40 +0100
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> 
> > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > With kernel header updates fuse_setxattr_in struct has grown in size.
> > > But this new struct size only takes affect if user has opted in
> > > for fuse feature FUSE_SETXATTR_EXT otherwise fuse continues to
> > > send "fuse_setxattr_in" of older size. Older size is determined
> > > by FUSE_COMPAT_SETXATTR_IN_SIZE.
> > > 
> > > Fix this. If we have not opted in for FUSE_SETXATTR_EXT, then
> > > expect that we will get fuse_setxattr_in of size FUSE_COMPAT_SETXATTR_IN_SIZE
> > > and not sizeof(struct fuse_sexattr_in).
> > > 
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > 
> > Yeh it's a bit of a grim fix, but I think it's the best we can do, and
> > we need to get it in since the headers have already been merged.
> 
> You can also add:
> 
> Fixes: 278f064e4524 ("Update Linux headers to 5.13-rc4")
> 
> because this is basically what happened : this commit exposes the API
> breakage.
> 
> This is kinda problematic : linux kernel headers are updated globally,
> i.e. an header update merged by some other subsystem will unknowingly
> break virtiofsd each time we face a similar situation.
> 
> We could cope with it if the code was adapted to API changes when
> needed, e.g. this patch squashed into 278f064e4524 . It doesn't
> seem that can be achievable without some automation to detect
> buggy situations (e.g. code depends on the size of a structure).
> And even with that, it would still cause the subsystem that
> needs the header update to depend on other subsystems to
> fix the breakage.
> 
> Another possibility could maybe to stop doing global updates and
> let each subsystem handle the kernel headers it needs.
> 
> OR we could avoid breaking the API in the kernel in the first
> place.

That would be my preference!

Dave

> Thoughts ?
> 
> Anyway, the fix is good:
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> > (I don't think libfuse has a fix for this in yet itself, but it might
> > survive because it doesn't bother copying the data like we do with
> > mbuf).
> > 
> > 
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > 
> > > ---
> > >  tools/virtiofsd/fuse_common.h   | 5 +++++
> > >  tools/virtiofsd/fuse_lowlevel.c | 7 ++++++-
> > >  2 files changed, 11 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/virtiofsd/fuse_common.h b/tools/virtiofsd/fuse_common.h
> > > index fa9671872e..0c2665b977 100644
> > > --- a/tools/virtiofsd/fuse_common.h
> > > +++ b/tools/virtiofsd/fuse_common.h
> > > @@ -372,6 +372,11 @@ struct fuse_file_info {
> > >   */
> > >  #define FUSE_CAP_HANDLE_KILLPRIV_V2 (1 << 28)
> > >  
> > > +/**
> > > + * Indicates that file server supports extended struct fuse_setxattr_in
> > > + */
> > > +#define FUSE_CAP_SETXATTR_EXT (1 << 29)
> > > +
> > >  /**
> > >   * Ioctl flags
> > >   *
> > > diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
> > > index 7fe2cef1eb..c2b6ff1686 100644
> > > --- a/tools/virtiofsd/fuse_lowlevel.c
> > > +++ b/tools/virtiofsd/fuse_lowlevel.c
> > > @@ -1419,8 +1419,13 @@ static void do_setxattr(fuse_req_t req, fuse_ino_t nodeid,
> > >      struct fuse_setxattr_in *arg;
> > >      const char *name;
> > >      const char *value;
> > > +    bool setxattr_ext = req->se->conn.want & FUSE_CAP_SETXATTR_EXT;
> > >  
> > > -    arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
> > > +    if (setxattr_ext) {
> > > +        arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
> > > +    } else {
> > > +        arg = fuse_mbuf_iter_advance(iter, FUSE_COMPAT_SETXATTR_IN_SIZE);
> > > +    }
> > >      name = fuse_mbuf_iter_advance_str(iter);
> > >      if (!arg || !name) {
> > >          fuse_reply_err(req, EINVAL);
> > > -- 
> > > 2.25.4
> > > 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v7 0/7] virtiofsd: Add support to enable/disable posix acls

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> Hi,
> 
> This is V7 of the patches.
> 
> Changes since V6.
> 
> - Dropped kernel header update patch as somebody else did it.
> - Fixed coding style issues.
> 
> Currently posix ACL support does not work well with virtiofs and bunch
> of tests fail when I run xfstests "./check -g acl".
> 
> This patches series fixes the issues with virtiofs posix acl support
> and provides options to enable/disable posix acl (-o posix_acl/no_posix_acl).
> By default posix_acls are disabled.
> 
> With this patch series applied and virtiofsd running with "-o posix_acl",
> xfstests "./check -g acl" passes.
> 
> Thanks
> Vivek

Queued

> 
> 
> Vivek Goyal (7):
>   virtiofsd: Fix fuse setxattr() API change issue
>   virtiofsd: Fix xattr operations overwriting errno
>   virtiofsd: Add support for extended setxattr
>   virtiofsd: Add umask to seccom allow list
>   virtiofsd: Add capability to change/restore umask
>   virtiofsd: Switch creds, drop FSETID for system.posix_acl_access xattr
>   virtiofsd: Add an option to enable/disable posix acls
> 
>  docs/tools/virtiofsd.rst              |   3 +
>  tools/virtiofsd/fuse_common.h         |  10 ++
>  tools/virtiofsd/fuse_lowlevel.c       |  18 +-
>  tools/virtiofsd/fuse_lowlevel.h       |   3 +-
>  tools/virtiofsd/helper.c              |   1 +
>  tools/virtiofsd/passthrough_ll.c      | 229 ++++++++++++++++++++++++--
>  tools/virtiofsd/passthrough_seccomp.c |   1 +
>  7 files changed, 249 insertions(+), 16 deletions(-)
> 
> -- 
> 2.25.4
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH v7 0/7] virtiofsd: Add support to enable/disable posix acls

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> Hi,
> 
> This is V7 of the patches.
> 
> Changes since V6.
> 
> - Dropped kernel header update patch as somebody else did it.
> - Fixed coding style issues.
> 
> Currently posix ACL support does not work well with virtiofs and bunch
> of tests fail when I run xfstests "./check -g acl".
> 
> This patches series fixes the issues with virtiofs posix acl support
> and provides options to enable/disable posix acl (-o posix_acl/no_posix_acl).
> By default posix_acls are disabled.
> 
> With this patch series applied and virtiofsd running with "-o posix_acl",
> xfstests "./check -g acl" passes.
> 
> Thanks
> Vivek

Queued

> 
> 
> Vivek Goyal (7):
>   virtiofsd: Fix fuse setxattr() API change issue
>   virtiofsd: Fix xattr operations overwriting errno
>   virtiofsd: Add support for extended setxattr
>   virtiofsd: Add umask to seccom allow list
>   virtiofsd: Add capability to change/restore umask
>   virtiofsd: Switch creds, drop FSETID for system.posix_acl_access xattr
>   virtiofsd: Add an option to enable/disable posix acls
> 
>  docs/tools/virtiofsd.rst              |   3 +
>  tools/virtiofsd/fuse_common.h         |  10 ++
>  tools/virtiofsd/fuse_lowlevel.c       |  18 +-
>  tools/virtiofsd/fuse_lowlevel.h       |   3 +-
>  tools/virtiofsd/helper.c              |   1 +
>  tools/virtiofsd/passthrough_ll.c      | 229 ++++++++++++++++++++++++--
>  tools/virtiofsd/passthrough_seccomp.c |   1 +
>  7 files changed, 249 insertions(+), 16 deletions(-)
> 
> -- 
> 2.25.4
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK