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

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

[Vivek Goyal]

Hi,

This is V2 of the patches. Changes since v1 are.

- Rebased on top of latest master.
- Took care of Miklos's comments to block acl xattrs if user
  explicitly disabled posix acl.

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

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

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

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

 tools/virtiofsd/passthrough_ll.c      | 119 ++++++++++++++++++++++++--
 tools/virtiofsd/passthrough_seccomp.c |   1 +
 2 files changed, 113 insertions(+), 7 deletions(-)

-- 
2.25.4

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

[Vivek Goyal]

Hi,

This is V2 of the patches. Changes since v1 are.

- Rebased on top of latest master.
- Took care of Miklos's comments to block acl xattrs if user
  explicitly disabled posix acl.

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

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

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

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

 tools/virtiofsd/passthrough_ll.c      | 119 ++++++++++++++++++++++++--
 tools/virtiofsd/passthrough_seccomp.c |   1 +
 2 files changed, 113 insertions(+), 7 deletions(-)

-- 
2.25.4

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

[Vivek Goyal]

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

Add virtiofsd option "-o posix_acl/no_posix_acl" to let users enable/disable
posix acl support. By default it is disabled as of now.

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.

v2: block system.posix_acl_access and system.posix_acl_default xattrs
    if user explicitly disabled acls. (Miklos)

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

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 58d24c0010..26cdfbd1f0 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -169,6 +169,7 @@ struct lo_data {
     /* An O_PATH file descriptor to /proc/self/fd/ */
     int proc_self_fd;
     int user_killpriv_v2, killpriv_v2;
+    int user_posix_acl;
 };
 
 static const struct fuse_opt lo_opts[] = {
@@ -201,6 +202,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;
@@ -661,6 +664,23 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn)
         conn->want &= ~FUSE_CAP_HANDLE_KILLPRIV_V2;
         lo->killpriv_v2 = 0;
     }
+
+    if (lo->user_posix_acl == 1) {
+        /*
+         * User explicitly asked for this option. Enable it unconditionally.
+         * If connection does not have this capability, it should fail
+         * in fuse_lowlevel.c
+         */
+        fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling posix acl\n");
+        conn->want |= FUSE_CAP_POSIX_ACL;
+    } else {
+        /*
+         * Either user specified to disable posix_acl, or did not specify
+         * anything. In both the cases do not enable posix acl.
+         */
+        fuse_log(FUSE_LOG_DEBUG, "lo_init: disabling posix_acl\n");
+        conn->want &= ~FUSE_CAP_POSIX_ACL;
+    }
 }
 
 static void lo_getattr(fuse_req_t req, fuse_ino_t ino,
@@ -2612,6 +2632,63 @@ static int xattr_map_server(const struct lo_data *lo, const char *server_name,
     return -ENODATA;
 }
 
+static bool block_xattr(struct lo_data *lo, const char *name)
+{
+    /*
+     * If user explicitly enabled posix_acl or did not provide any option,
+     * do not block acl. Otherwise block system.posix_acl_access and
+     * system.posix_acl_default xattrs.
+     */
+    if (lo->user_posix_acl) {
+        return false;
+    }
+    if (!strcmp(name, "system.posix_acl_access") ||
+        !strcmp(name, "system.posix_acl_default"))
+            return true;
+
+    return false;
+}
+
+/*
+ * Returns number of bytes in xattr_list after filtering on success. This
+ * could be zero as well if nothing is left after filtering.
+ *
+ * Returns negative error code on failure.
+ * xattr_list is modified in place.
+ */
+static int remove_blocked_xattrs(struct lo_data *lo, char *xattr_list,
+                                 unsigned in_size)
+{
+    size_t out_index, in_index;
+
+    /*
+     * As of now we only filter out acl xattrs. If acls are enabled or
+     * they have not been explicitly disabled, there is nothing to
+     * filter.
+     */
+    if (lo->user_posix_acl) {
+        return in_size;
+    }
+
+    out_index = 0;
+    in_index = 0;
+    while (in_index < in_size) {
+        char *in_ptr = xattr_list + in_index;
+
+        /* Length of current attribute name */
+        size_t in_len = strlen(xattr_list + in_index) + 1;
+
+        if (!block_xattr(lo, in_ptr)) {
+            if (in_index != out_index) {
+                memmove(xattr_list + out_index, xattr_list + in_index, in_len);
+            }
+            out_index += in_len;
+        }
+        in_index += in_len;
+     }
+    return out_index;
+}
+
 static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
                         size_t size)
 {
@@ -2625,6 +2702,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) {
@@ -2766,7 +2848,6 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
         if (ret == 0) {
             goto out;
         }
-
         if (lo->xattr_map_list) {
             /*
              * Map the names back, some attributes might be dropped,
@@ -2813,6 +2894,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 {
         /*
@@ -2851,6 +2938,11 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
     int saverr;
     int fd = -1;
 
+    if (block_xattr(lo, in_name)) {
+        fuse_reply_err(req, EOPNOTSUPP);
+        return;
+    }
+
     mapped_name = NULL;
     name = in_name;
     if (lo->xattrmap) {
@@ -3604,6 +3696,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;
-- 
2.25.4

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

[Vivek Goyal]

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

Add virtiofsd option "-o posix_acl/no_posix_acl" to let users enable/disable
posix acl support. By default it is disabled as of now.

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.

v2: block system.posix_acl_access and system.posix_acl_default xattrs
    if user explicitly disabled acls. (Miklos)

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

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 58d24c0010..26cdfbd1f0 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -169,6 +169,7 @@ struct lo_data {
     /* An O_PATH file descriptor to /proc/self/fd/ */
     int proc_self_fd;
     int user_killpriv_v2, killpriv_v2;
+    int user_posix_acl;
 };
 
 static const struct fuse_opt lo_opts[] = {
@@ -201,6 +202,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;
@@ -661,6 +664,23 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn)
         conn->want &= ~FUSE_CAP_HANDLE_KILLPRIV_V2;
         lo->killpriv_v2 = 0;
     }
+
+    if (lo->user_posix_acl == 1) {
+        /*
+         * User explicitly asked for this option. Enable it unconditionally.
+         * If connection does not have this capability, it should fail
+         * in fuse_lowlevel.c
+         */
+        fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling posix acl\n");
+        conn->want |= FUSE_CAP_POSIX_ACL;
+    } else {
+        /*
+         * Either user specified to disable posix_acl, or did not specify
+         * anything. In both the cases do not enable posix acl.
+         */
+        fuse_log(FUSE_LOG_DEBUG, "lo_init: disabling posix_acl\n");
+        conn->want &= ~FUSE_CAP_POSIX_ACL;
+    }
 }
 
 static void lo_getattr(fuse_req_t req, fuse_ino_t ino,
@@ -2612,6 +2632,63 @@ static int xattr_map_server(const struct lo_data *lo, const char *server_name,
     return -ENODATA;
 }
 
+static bool block_xattr(struct lo_data *lo, const char *name)
+{
+    /*
+     * If user explicitly enabled posix_acl or did not provide any option,
+     * do not block acl. Otherwise block system.posix_acl_access and
+     * system.posix_acl_default xattrs.
+     */
+    if (lo->user_posix_acl) {
+        return false;
+    }
+    if (!strcmp(name, "system.posix_acl_access") ||
+        !strcmp(name, "system.posix_acl_default"))
+            return true;
+
+    return false;
+}
+
+/*
+ * Returns number of bytes in xattr_list after filtering on success. This
+ * could be zero as well if nothing is left after filtering.
+ *
+ * Returns negative error code on failure.
+ * xattr_list is modified in place.
+ */
+static int remove_blocked_xattrs(struct lo_data *lo, char *xattr_list,
+                                 unsigned in_size)
+{
+    size_t out_index, in_index;
+
+    /*
+     * As of now we only filter out acl xattrs. If acls are enabled or
+     * they have not been explicitly disabled, there is nothing to
+     * filter.
+     */
+    if (lo->user_posix_acl) {
+        return in_size;
+    }
+
+    out_index = 0;
+    in_index = 0;
+    while (in_index < in_size) {
+        char *in_ptr = xattr_list + in_index;
+
+        /* Length of current attribute name */
+        size_t in_len = strlen(xattr_list + in_index) + 1;
+
+        if (!block_xattr(lo, in_ptr)) {
+            if (in_index != out_index) {
+                memmove(xattr_list + out_index, xattr_list + in_index, in_len);
+            }
+            out_index += in_len;
+        }
+        in_index += in_len;
+     }
+    return out_index;
+}
+
 static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
                         size_t size)
 {
@@ -2625,6 +2702,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) {
@@ -2766,7 +2848,6 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
         if (ret == 0) {
             goto out;
         }
-
         if (lo->xattr_map_list) {
             /*
              * Map the names back, some attributes might be dropped,
@@ -2813,6 +2894,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 {
         /*
@@ -2851,6 +2938,11 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
     int saverr;
     int fd = -1;
 
+    if (block_xattr(lo, in_name)) {
+        fuse_reply_err(req, EOPNOTSUPP);
+        return;
+    }
+
     mapped_name = NULL;
     name = in_name;
     if (lo->xattrmap) {
@@ -3604,6 +3696,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;
-- 
2.25.4

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

[Vivek Goyal]

On Wed, Feb 17, 2021 at 06:30:44PM -0500, Vivek Goyal wrote:
> fuse has an option FUSE_POSIX_ACL which needs to be opted in by fuse
> server to enable posix acls.
> 
> Add virtiofsd option "-o posix_acl/no_posix_acl" to let users enable/disable
> posix acl support. By default it is disabled as of now.
> 
> 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.

I forgot to block acl xattrs in lo_removexattr(). Will respin this patch.

Vivek

> 
> 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.
> 
> v2: block system.posix_acl_access and system.posix_acl_default xattrs
>     if user explicitly disabled acls. (Miklos)
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  tools/virtiofsd/passthrough_ll.c | 95 +++++++++++++++++++++++++++++++-
>  1 file changed, 94 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 58d24c0010..26cdfbd1f0 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -169,6 +169,7 @@ struct lo_data {
>      /* An O_PATH file descriptor to /proc/self/fd/ */
>      int proc_self_fd;
>      int user_killpriv_v2, killpriv_v2;
> +    int user_posix_acl;
>  };
>  
>  static const struct fuse_opt lo_opts[] = {
> @@ -201,6 +202,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;
> @@ -661,6 +664,23 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn)
>          conn->want &= ~FUSE_CAP_HANDLE_KILLPRIV_V2;
>          lo->killpriv_v2 = 0;
>      }
> +
> +    if (lo->user_posix_acl == 1) {
> +        /*
> +         * User explicitly asked for this option. Enable it unconditionally.
> +         * If connection does not have this capability, it should fail
> +         * in fuse_lowlevel.c
> +         */
> +        fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling posix acl\n");
> +        conn->want |= FUSE_CAP_POSIX_ACL;
> +    } else {
> +        /*
> +         * Either user specified to disable posix_acl, or did not specify
> +         * anything. In both the cases do not enable posix acl.
> +         */
> +        fuse_log(FUSE_LOG_DEBUG, "lo_init: disabling posix_acl\n");
> +        conn->want &= ~FUSE_CAP_POSIX_ACL;
> +    }
>  }
>  
>  static void lo_getattr(fuse_req_t req, fuse_ino_t ino,
> @@ -2612,6 +2632,63 @@ static int xattr_map_server(const struct lo_data *lo, const char *server_name,
>      return -ENODATA;
>  }
>  
> +static bool block_xattr(struct lo_data *lo, const char *name)
> +{
> +    /*
> +     * If user explicitly enabled posix_acl or did not provide any option,
> +     * do not block acl. Otherwise block system.posix_acl_access and
> +     * system.posix_acl_default xattrs.
> +     */
> +    if (lo->user_posix_acl) {
> +        return false;
> +    }
> +    if (!strcmp(name, "system.posix_acl_access") ||
> +        !strcmp(name, "system.posix_acl_default"))
> +            return true;
> +
> +    return false;
> +}
> +
> +/*
> + * Returns number of bytes in xattr_list after filtering on success. This
> + * could be zero as well if nothing is left after filtering.
> + *
> + * Returns negative error code on failure.
> + * xattr_list is modified in place.
> + */
> +static int remove_blocked_xattrs(struct lo_data *lo, char *xattr_list,
> +                                 unsigned in_size)
> +{
> +    size_t out_index, in_index;
> +
> +    /*
> +     * As of now we only filter out acl xattrs. If acls are enabled or
> +     * they have not been explicitly disabled, there is nothing to
> +     * filter.
> +     */
> +    if (lo->user_posix_acl) {
> +        return in_size;
> +    }
> +
> +    out_index = 0;
> +    in_index = 0;
> +    while (in_index < in_size) {
> +        char *in_ptr = xattr_list + in_index;
> +
> +        /* Length of current attribute name */
> +        size_t in_len = strlen(xattr_list + in_index) + 1;
> +
> +        if (!block_xattr(lo, in_ptr)) {
> +            if (in_index != out_index) {
> +                memmove(xattr_list + out_index, xattr_list + in_index, in_len);
> +            }
> +            out_index += in_len;
> +        }
> +        in_index += in_len;
> +     }
> +    return out_index;
> +}
> +
>  static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
>                          size_t size)
>  {
> @@ -2625,6 +2702,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) {
> @@ -2766,7 +2848,6 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
>          if (ret == 0) {
>              goto out;
>          }
> -
>          if (lo->xattr_map_list) {
>              /*
>               * Map the names back, some attributes might be dropped,
> @@ -2813,6 +2894,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 {
>          /*
> @@ -2851,6 +2938,11 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
>      int saverr;
>      int fd = -1;
>  
> +    if (block_xattr(lo, in_name)) {
> +        fuse_reply_err(req, EOPNOTSUPP);
> +        return;
> +    }
> +
>      mapped_name = NULL;
>      name = in_name;
>      if (lo->xattrmap) {
> @@ -3604,6 +3696,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;
> -- 
> 2.25.4
> 

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

[Vivek Goyal]

On Wed, Feb 17, 2021 at 06:30:44PM -0500, Vivek Goyal wrote:
> fuse has an option FUSE_POSIX_ACL which needs to be opted in by fuse
> server to enable posix acls.
> 
> Add virtiofsd option "-o posix_acl/no_posix_acl" to let users enable/disable
> posix acl support. By default it is disabled as of now.
> 
> 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.

I forgot to block acl xattrs in lo_removexattr(). Will respin this patch.

Vivek

> 
> 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.
> 
> v2: block system.posix_acl_access and system.posix_acl_default xattrs
>     if user explicitly disabled acls. (Miklos)
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  tools/virtiofsd/passthrough_ll.c | 95 +++++++++++++++++++++++++++++++-
>  1 file changed, 94 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 58d24c0010..26cdfbd1f0 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -169,6 +169,7 @@ struct lo_data {
>      /* An O_PATH file descriptor to /proc/self/fd/ */
>      int proc_self_fd;
>      int user_killpriv_v2, killpriv_v2;
> +    int user_posix_acl;
>  };
>  
>  static const struct fuse_opt lo_opts[] = {
> @@ -201,6 +202,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;
> @@ -661,6 +664,23 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn)
>          conn->want &= ~FUSE_CAP_HANDLE_KILLPRIV_V2;
>          lo->killpriv_v2 = 0;
>      }
> +
> +    if (lo->user_posix_acl == 1) {
> +        /*
> +         * User explicitly asked for this option. Enable it unconditionally.
> +         * If connection does not have this capability, it should fail
> +         * in fuse_lowlevel.c
> +         */
> +        fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling posix acl\n");
> +        conn->want |= FUSE_CAP_POSIX_ACL;
> +    } else {
> +        /*
> +         * Either user specified to disable posix_acl, or did not specify
> +         * anything. In both the cases do not enable posix acl.
> +         */
> +        fuse_log(FUSE_LOG_DEBUG, "lo_init: disabling posix_acl\n");
> +        conn->want &= ~FUSE_CAP_POSIX_ACL;
> +    }
>  }
>  
>  static void lo_getattr(fuse_req_t req, fuse_ino_t ino,
> @@ -2612,6 +2632,63 @@ static int xattr_map_server(const struct lo_data *lo, const char *server_name,
>      return -ENODATA;
>  }
>  
> +static bool block_xattr(struct lo_data *lo, const char *name)
> +{
> +    /*
> +     * If user explicitly enabled posix_acl or did not provide any option,
> +     * do not block acl. Otherwise block system.posix_acl_access and
> +     * system.posix_acl_default xattrs.
> +     */
> +    if (lo->user_posix_acl) {
> +        return false;
> +    }
> +    if (!strcmp(name, "system.posix_acl_access") ||
> +        !strcmp(name, "system.posix_acl_default"))
> +            return true;
> +
> +    return false;
> +}
> +
> +/*
> + * Returns number of bytes in xattr_list after filtering on success. This
> + * could be zero as well if nothing is left after filtering.
> + *
> + * Returns negative error code on failure.
> + * xattr_list is modified in place.
> + */
> +static int remove_blocked_xattrs(struct lo_data *lo, char *xattr_list,
> +                                 unsigned in_size)
> +{
> +    size_t out_index, in_index;
> +
> +    /*
> +     * As of now we only filter out acl xattrs. If acls are enabled or
> +     * they have not been explicitly disabled, there is nothing to
> +     * filter.
> +     */
> +    if (lo->user_posix_acl) {
> +        return in_size;
> +    }
> +
> +    out_index = 0;
> +    in_index = 0;
> +    while (in_index < in_size) {
> +        char *in_ptr = xattr_list + in_index;
> +
> +        /* Length of current attribute name */
> +        size_t in_len = strlen(xattr_list + in_index) + 1;
> +
> +        if (!block_xattr(lo, in_ptr)) {
> +            if (in_index != out_index) {
> +                memmove(xattr_list + out_index, xattr_list + in_index, in_len);
> +            }
> +            out_index += in_len;
> +        }
> +        in_index += in_len;
> +     }
> +    return out_index;
> +}
> +
>  static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
>                          size_t size)
>  {
> @@ -2625,6 +2702,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) {
> @@ -2766,7 +2848,6 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
>          if (ret == 0) {
>              goto out;
>          }
> -
>          if (lo->xattr_map_list) {
>              /*
>               * Map the names back, some attributes might be dropped,
> @@ -2813,6 +2894,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 {
>          /*
> @@ -2851,6 +2938,11 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
>      int saverr;
>      int fd = -1;
>  
> +    if (block_xattr(lo, in_name)) {
> +        fuse_reply_err(req, EOPNOTSUPP);
> +        return;
> +    }
> +
>      mapped_name = NULL;
>      name = in_name;
>      if (lo->xattrmap) {
> @@ -3604,6 +3696,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;
> -- 
> 2.25.4
> 

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

[Vivek Goyal]

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

Add virtiofsd option "-o posix_acl/no_posix_acl" to let users enable/disable
posix acl support. By default it is disabled as of now.

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. 

v2.1: Fixed lo_removexattr()

v2: block system.posix_acl_access and system.posix_acl_default xattrs
    if user explicitly disabled acls. (Miklos)

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

Index: rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c
===================================================================
--- rhvgoyal-qemu.orig/tools/virtiofsd/passthrough_ll.c	2021-02-18 13:12:26.839770339 -0500
+++ rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c	2021-02-18 13:38:22.344930118 -0500
@@ -169,6 +169,7 @@ struct lo_data {
     /* An O_PATH file descriptor to /proc/self/fd/ */
     int proc_self_fd;
     int user_killpriv_v2, killpriv_v2;
+    int user_posix_acl;
 };
 
 static const struct fuse_opt lo_opts[] = {
@@ -201,6 +202,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;
@@ -661,6 +664,23 @@ static void lo_init(void *userdata, stru
         conn->want &= ~FUSE_CAP_HANDLE_KILLPRIV_V2;
         lo->killpriv_v2 = 0;
     }
+
+    if (lo->user_posix_acl == 1) {
+        /*
+         * User explicitly asked for this option. Enable it unconditionally.
+         * If connection does not have this capability, it should fail
+         * in fuse_lowlevel.c
+         */
+        fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling posix acl\n");
+        conn->want |= FUSE_CAP_POSIX_ACL;
+    } else {
+        /*
+         * Either user specified to disable posix_acl, or did not specify
+         * anything. In both the cases do not enable posix acl.
+         */
+        fuse_log(FUSE_LOG_DEBUG, "lo_init: disabling posix_acl\n");
+        conn->want &= ~FUSE_CAP_POSIX_ACL;
+    }
 }
 
 static void lo_getattr(fuse_req_t req, fuse_ino_t ino,
@@ -2612,6 +2632,63 @@ static int xattr_map_server(const struct
     return -ENODATA;
 }
 
+static bool block_xattr(struct lo_data *lo, const char *name)
+{
+    /*
+     * If user explicitly enabled posix_acl or did not provide any option,
+     * do not block acl. Otherwise block system.posix_acl_access and
+     * system.posix_acl_default xattrs.
+     */
+    if (lo->user_posix_acl) {
+        return false;
+    }
+    if (!strcmp(name, "system.posix_acl_access") ||
+        !strcmp(name, "system.posix_acl_default"))
+            return true;
+
+    return false;
+}
+
+/*
+ * Returns number of bytes in xattr_list after filtering on success. This
+ * could be zero as well if nothing is left after filtering.
+ *
+ * Returns negative error code on failure.
+ * xattr_list is modified in place.
+ */
+static int remove_blocked_xattrs(struct lo_data *lo, char *xattr_list,
+                                 unsigned in_size)
+{
+    size_t out_index, in_index;
+
+    /*
+     * As of now we only filter out acl xattrs. If acls are enabled or
+     * they have not been explicitly disabled, there is nothing to
+     * filter.
+     */
+    if (lo->user_posix_acl) {
+        return in_size;
+    }
+
+    out_index = 0;
+    in_index = 0;
+    while (in_index < in_size) {
+        char *in_ptr = xattr_list + in_index;
+
+        /* Length of current attribute name */
+        size_t in_len = strlen(xattr_list + in_index) + 1;
+
+        if (!block_xattr(lo, in_ptr)) {
+            if (in_index != out_index) {
+                memmove(xattr_list + out_index, xattr_list + in_index, in_len);
+            }
+            out_index += in_len;
+        }
+        in_index += in_len;
+     }
+    return out_index;
+}
+
 static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
                         size_t size)
 {
@@ -2625,6 +2702,11 @@ static void lo_getxattr(fuse_req_t req,
     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) {
@@ -2766,7 +2848,6 @@ static void lo_listxattr(fuse_req_t req,
         if (ret == 0) {
             goto out;
         }
-
         if (lo->xattr_map_list) {
             /*
              * Map the names back, some attributes might be dropped,
@@ -2813,6 +2894,12 @@ static void lo_listxattr(fuse_req_t req,
                 goto out;
             }
         }
+
+        ret = remove_blocked_xattrs(lo, value, ret);
+        if (ret <= 0) {
+            saverr = -ret;
+            goto out;
+        }
         fuse_reply_buf(req, value, ret);
     } else {
         /*
@@ -2851,6 +2938,11 @@ static void lo_setxattr(fuse_req_t req,
     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) {
@@ -2917,6 +3009,11 @@ static void lo_removexattr(fuse_req_t re
     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) {
@@ -3604,6 +3701,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;

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

[Vivek Goyal]

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

Add virtiofsd option "-o posix_acl/no_posix_acl" to let users enable/disable
posix acl support. By default it is disabled as of now.

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. 

v2.1: Fixed lo_removexattr()

v2: block system.posix_acl_access and system.posix_acl_default xattrs
    if user explicitly disabled acls. (Miklos)

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

Index: rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c
===================================================================
--- rhvgoyal-qemu.orig/tools/virtiofsd/passthrough_ll.c	2021-02-18 13:12:26.839770339 -0500
+++ rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c	2021-02-18 13:38:22.344930118 -0500
@@ -169,6 +169,7 @@ struct lo_data {
     /* An O_PATH file descriptor to /proc/self/fd/ */
     int proc_self_fd;
     int user_killpriv_v2, killpriv_v2;
+    int user_posix_acl;
 };
 
 static const struct fuse_opt lo_opts[] = {
@@ -201,6 +202,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;
@@ -661,6 +664,23 @@ static void lo_init(void *userdata, stru
         conn->want &= ~FUSE_CAP_HANDLE_KILLPRIV_V2;
         lo->killpriv_v2 = 0;
     }
+
+    if (lo->user_posix_acl == 1) {
+        /*
+         * User explicitly asked for this option. Enable it unconditionally.
+         * If connection does not have this capability, it should fail
+         * in fuse_lowlevel.c
+         */
+        fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling posix acl\n");
+        conn->want |= FUSE_CAP_POSIX_ACL;
+    } else {
+        /*
+         * Either user specified to disable posix_acl, or did not specify
+         * anything. In both the cases do not enable posix acl.
+         */
+        fuse_log(FUSE_LOG_DEBUG, "lo_init: disabling posix_acl\n");
+        conn->want &= ~FUSE_CAP_POSIX_ACL;
+    }
 }
 
 static void lo_getattr(fuse_req_t req, fuse_ino_t ino,
@@ -2612,6 +2632,63 @@ static int xattr_map_server(const struct
     return -ENODATA;
 }
 
+static bool block_xattr(struct lo_data *lo, const char *name)
+{
+    /*
+     * If user explicitly enabled posix_acl or did not provide any option,
+     * do not block acl. Otherwise block system.posix_acl_access and
+     * system.posix_acl_default xattrs.
+     */
+    if (lo->user_posix_acl) {
+        return false;
+    }
+    if (!strcmp(name, "system.posix_acl_access") ||
+        !strcmp(name, "system.posix_acl_default"))
+            return true;
+
+    return false;
+}
+
+/*
+ * Returns number of bytes in xattr_list after filtering on success. This
+ * could be zero as well if nothing is left after filtering.
+ *
+ * Returns negative error code on failure.
+ * xattr_list is modified in place.
+ */
+static int remove_blocked_xattrs(struct lo_data *lo, char *xattr_list,
+                                 unsigned in_size)
+{
+    size_t out_index, in_index;
+
+    /*
+     * As of now we only filter out acl xattrs. If acls are enabled or
+     * they have not been explicitly disabled, there is nothing to
+     * filter.
+     */
+    if (lo->user_posix_acl) {
+        return in_size;
+    }
+
+    out_index = 0;
+    in_index = 0;
+    while (in_index < in_size) {
+        char *in_ptr = xattr_list + in_index;
+
+        /* Length of current attribute name */
+        size_t in_len = strlen(xattr_list + in_index) + 1;
+
+        if (!block_xattr(lo, in_ptr)) {
+            if (in_index != out_index) {
+                memmove(xattr_list + out_index, xattr_list + in_index, in_len);
+            }
+            out_index += in_len;
+        }
+        in_index += in_len;
+     }
+    return out_index;
+}
+
 static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
                         size_t size)
 {
@@ -2625,6 +2702,11 @@ static void lo_getxattr(fuse_req_t req,
     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) {
@@ -2766,7 +2848,6 @@ static void lo_listxattr(fuse_req_t req,
         if (ret == 0) {
             goto out;
         }
-
         if (lo->xattr_map_list) {
             /*
              * Map the names back, some attributes might be dropped,
@@ -2813,6 +2894,12 @@ static void lo_listxattr(fuse_req_t req,
                 goto out;
             }
         }
+
+        ret = remove_blocked_xattrs(lo, value, ret);
+        if (ret <= 0) {
+            saverr = -ret;
+            goto out;
+        }
         fuse_reply_buf(req, value, ret);
     } else {
         /*
@@ -2851,6 +2938,11 @@ static void lo_setxattr(fuse_req_t req,
     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) {
@@ -2917,6 +3009,11 @@ static void lo_removexattr(fuse_req_t re
     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) {
@@ -3604,6 +3701,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;

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

[Vivek Goyal]

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

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

diff --git a/tools/virtiofsd/passthrough_seccomp.c b/tools/virtiofsd/passthrough_seccomp.c
index 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 v2 2/3] virtiofsd: Add umask to seccom allow list

[Vivek Goyal]

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

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

diff --git a/tools/virtiofsd/passthrough_seccomp.c b/tools/virtiofsd/passthrough_seccomp.c
index 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 v2 3/3] virtiofsd: Change umask if posix acls are enabled

[Vivek Goyal]

When parent directory has default acl and a file is created in that
directory, then umask is ignored and final file permissions are
determined using default acl instead. (man 2 umask).

Currently, fuse applies the umask and sends modified mode in create
request accordingly. fuse server can set FUSE_DONT_MASK and tell
fuse client to not apply umask and fuse server will take care of
it as needed.

With posix acls enabled, requirement will be that we want umask
to determine final file mode if parent directory does not have
default acl.

So if posix acls are enabled, opt in for FUSE_DONT_MASK. virtiofsd
will set umask of the thread doing file creation. And host kernel
should use that umask if parent directory does not have default
acls, otherwise umask does not take affect.

Miklos mentioned that we already call unshare(CLONE_FS) for
every thread. That means umask has now become property of per
thread and it should be ok to manipulate it in file creation path.

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

This should fix fstest generic/099.

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

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 26cdfbd1f0..f92737b7bb 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -120,6 +120,7 @@ struct lo_inode {
 struct lo_cred {
     uid_t euid;
     gid_t egid;
+    mode_t umask;
 };
 
 enum {
@@ -170,6 +171,8 @@ struct lo_data {
     int proc_self_fd;
     int user_killpriv_v2, killpriv_v2;
     int user_posix_acl;
+    /* If set, virtiofsd is responsible for setting umask during creation */
+    bool change_umask;
 };
 
 static const struct fuse_opt lo_opts[] = {
@@ -672,7 +675,8 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn)
          * in fuse_lowlevel.c
          */
         fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling posix acl\n");
-        conn->want |= FUSE_CAP_POSIX_ACL;
+        conn->want |= FUSE_CAP_POSIX_ACL | FUSE_CAP_DONT_MASK;
+        lo->change_umask = true;
     } else {
         /*
          * Either user specified to disable posix_acl, or did not specify
@@ -680,6 +684,7 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn)
          */
         fuse_log(FUSE_LOG_DEBUG, "lo_init: disabling posix_acl\n");
         conn->want &= ~FUSE_CAP_POSIX_ACL;
+        lo->change_umask = false;
     }
 }
 
@@ -1095,7 +1100,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;
 
@@ -1115,11 +1121,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;
 
@@ -1134,6 +1143,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,
@@ -1158,7 +1170,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;
     }
@@ -1167,7 +1179,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;
@@ -1848,7 +1860,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;
     }
@@ -1859,7 +1871,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 v2 3/3] virtiofsd: Change umask if posix acls are enabled

[Vivek Goyal]

When parent directory has default acl and a file is created in that
directory, then umask is ignored and final file permissions are
determined using default acl instead. (man 2 umask).

Currently, fuse applies the umask and sends modified mode in create
request accordingly. fuse server can set FUSE_DONT_MASK and tell
fuse client to not apply umask and fuse server will take care of
it as needed.

With posix acls enabled, requirement will be that we want umask
to determine final file mode if parent directory does not have
default acl.

So if posix acls are enabled, opt in for FUSE_DONT_MASK. virtiofsd
will set umask of the thread doing file creation. And host kernel
should use that umask if parent directory does not have default
acls, otherwise umask does not take affect.

Miklos mentioned that we already call unshare(CLONE_FS) for
every thread. That means umask has now become property of per
thread and it should be ok to manipulate it in file creation path.

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

This should fix fstest generic/099.

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

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 26cdfbd1f0..f92737b7bb 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -120,6 +120,7 @@ struct lo_inode {
 struct lo_cred {
     uid_t euid;
     gid_t egid;
+    mode_t umask;
 };
 
 enum {
@@ -170,6 +171,8 @@ struct lo_data {
     int proc_self_fd;
     int user_killpriv_v2, killpriv_v2;
     int user_posix_acl;
+    /* If set, virtiofsd is responsible for setting umask during creation */
+    bool change_umask;
 };
 
 static const struct fuse_opt lo_opts[] = {
@@ -672,7 +675,8 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn)
          * in fuse_lowlevel.c
          */
         fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling posix acl\n");
-        conn->want |= FUSE_CAP_POSIX_ACL;
+        conn->want |= FUSE_CAP_POSIX_ACL | FUSE_CAP_DONT_MASK;
+        lo->change_umask = true;
     } else {
         /*
          * Either user specified to disable posix_acl, or did not specify
@@ -680,6 +684,7 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn)
          */
         fuse_log(FUSE_LOG_DEBUG, "lo_init: disabling posix_acl\n");
         conn->want &= ~FUSE_CAP_POSIX_ACL;
+        lo->change_umask = false;
     }
 }
 
@@ -1095,7 +1100,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;
 
@@ -1115,11 +1121,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;
 
@@ -1134,6 +1143,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,
@@ -1158,7 +1170,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;
     }
@@ -1167,7 +1179,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;
@@ -1848,7 +1860,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;
     }
@@ -1859,7 +1871,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
     fd = openat(parent_inode->fd, name, fi->flags | O_CREAT | O_EXCL, mode);
     err = fd == -1 ? errno : 0;
 
-    lo_restore_cred(&old);
+    lo_restore_cred(&old, lo->change_umask);
 
     /* Ignore the error if file exists and O_EXCL was not given */
     if (err && (err != EEXIST || (fi->flags & O_EXCL))) {
-- 
2.25.4

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

[Luis Henriques]

Vivek Goyal <vgoyal@redhat.com> writes:

> Hi,
>
> This is V2 of the patches. Changes since v1 are.
>
> - Rebased on top of latest master.
> - Took care of Miklos's comments to block acl xattrs if user
>   explicitly disabled posix acl.
>
> Luis Henriques reported that fstest generic/099 fails with virtiofs.
> Little debugging showed that we don't enable acl support. So this
> patch series provides option to enable/disable posix acl support. By
> default it is disabled.
>
> I have run blogbench and pjdfstests with posix acl enabled and
> things work fine.
>
> Luis, can you please apply these patches, and run virtiofsd with
> "-o posix_acl" and see if it fixes the failure you are seeing. I
> ran the steps you provided manually and it fixes the issue for
> me.

Sorry for the delay.  I've finally tested these patches and they indeed
fix the problem I reported.  My only question about this fix is why is
this option not enabled by default, since this is the documented behavior
in acl(5) and umask(2)?  In fact, why is this an option at all? 

Cheers,
-- 
Luis

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

[Luis Henriques]

Vivek Goyal <vgoyal@redhat.com> writes:

> Hi,
>
> This is V2 of the patches. Changes since v1 are.
>
> - Rebased on top of latest master.
> - Took care of Miklos's comments to block acl xattrs if user
>   explicitly disabled posix acl.
>
> Luis Henriques reported that fstest generic/099 fails with virtiofs.
> Little debugging showed that we don't enable acl support. So this
> patch series provides option to enable/disable posix acl support. By
> default it is disabled.
>
> I have run blogbench and pjdfstests with posix acl enabled and
> things work fine.
>
> Luis, can you please apply these patches, and run virtiofsd with
> "-o posix_acl" and see if it fixes the failure you are seeing. I
> ran the steps you provided manually and it fixes the issue for
> me.

Sorry for the delay.  I've finally tested these patches and they indeed
fix the problem I reported.  My only question about this fix is why is
this option not enabled by default, since this is the documented behavior
in acl(5) and umask(2)?  In fact, why is this an option at all? 

Cheers,
-- 
Luis

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

[Vivek Goyal]

On Fri, Feb 19, 2021 at 11:50:54AM +0000, Luis Henriques wrote:
> Vivek Goyal <vgoyal@redhat.com> writes:
> 
> > Hi,
> >
> > This is V2 of the patches. Changes since v1 are.
> >
> > - Rebased on top of latest master.
> > - Took care of Miklos's comments to block acl xattrs if user
> >   explicitly disabled posix acl.
> >
> > Luis Henriques reported that fstest generic/099 fails with virtiofs.
> > Little debugging showed that we don't enable acl support. So this
> > patch series provides option to enable/disable posix acl support. By
> > default it is disabled.
> >
> > I have run blogbench and pjdfstests with posix acl enabled and
> > things work fine.
> >
> > Luis, can you please apply these patches, and run virtiofsd with
> > "-o posix_acl" and see if it fixes the failure you are seeing. I
> > ran the steps you provided manually and it fixes the issue for
> > me.
> 
> Sorry for the delay.  I've finally tested these patches and they indeed
> fix the problem I reported.  My only question about this fix is why is
> this option not enabled by default, since this is the documented behavior
> in acl(5) and umask(2)?  In fact, why is this an option at all? 

You mean why to not enable acl by default?

I am concerned about performance drop this can lead to because extra
GETXATTR(system.posix_acl_*) messages which will trigger if acls are enabled.
And not all users might require these. That's why I preferred to not enable
acl by default. Those who need it can enable it explicitly.

Another example is xattr support. Due to performance concerns, we don't
enable xattrs by default either.

Thanks
Vivek

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

[Vivek Goyal]

On Fri, Feb 19, 2021 at 11:50:54AM +0000, Luis Henriques wrote:
> Vivek Goyal <vgoyal@redhat.com> writes:
> 
> > Hi,
> >
> > This is V2 of the patches. Changes since v1 are.
> >
> > - Rebased on top of latest master.
> > - Took care of Miklos's comments to block acl xattrs if user
> >   explicitly disabled posix acl.
> >
> > Luis Henriques reported that fstest generic/099 fails with virtiofs.
> > Little debugging showed that we don't enable acl support. So this
> > patch series provides option to enable/disable posix acl support. By
> > default it is disabled.
> >
> > I have run blogbench and pjdfstests with posix acl enabled and
> > things work fine.
> >
> > Luis, can you please apply these patches, and run virtiofsd with
> > "-o posix_acl" and see if it fixes the failure you are seeing. I
> > ran the steps you provided manually and it fixes the issue for
> > me.
> 
> Sorry for the delay.  I've finally tested these patches and they indeed
> fix the problem I reported.  My only question about this fix is why is
> this option not enabled by default, since this is the documented behavior
> in acl(5) and umask(2)?  In fact, why is this an option at all? 

You mean why to not enable acl by default?

I am concerned about performance drop this can lead to because extra
GETXATTR(system.posix_acl_*) messages which will trigger if acls are enabled.
And not all users might require these. That's why I preferred to not enable
acl by default. Those who need it can enable it explicitly.

Another example is xattr support. Due to performance concerns, we don't
enable xattrs by default either.

Thanks
Vivek

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

[Miklos Szeredi]

On Fri, Feb 19, 2021 at 3:34 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Fri, Feb 19, 2021 at 11:50:54AM +0000, Luis Henriques wrote:
> > Vivek Goyal <vgoyal@redhat.com> writes:
> >
> > > Hi,
> > >
> > > This is V2 of the patches. Changes since v1 are.
> > >
> > > - Rebased on top of latest master.
> > > - Took care of Miklos's comments to block acl xattrs if user
> > >   explicitly disabled posix acl.
> > >
> > > Luis Henriques reported that fstest generic/099 fails with virtiofs.
> > > Little debugging showed that we don't enable acl support. So this
> > > patch series provides option to enable/disable posix acl support. By
> > > default it is disabled.
> > >
> > > I have run blogbench and pjdfstests with posix acl enabled and
> > > things work fine.
> > >
> > > Luis, can you please apply these patches, and run virtiofsd with
> > > "-o posix_acl" and see if it fixes the failure you are seeing. I
> > > ran the steps you provided manually and it fixes the issue for
> > > me.
> >
> > Sorry for the delay.  I've finally tested these patches and they indeed
> > fix the problem I reported.  My only question about this fix is why is
> > this option not enabled by default, since this is the documented behavior
> > in acl(5) and umask(2)?  In fact, why is this an option at all?
>
> You mean why to not enable acl by default?
>
> I am concerned about performance drop this can lead to because extra
> GETXATTR(system.posix_acl_*) messages which will trigger if acls are enabled.
> And not all users might require these. That's why I preferred to not enable
> acl by default. Those who need it can enable it explicitly.
>
> Another example is xattr support. Due to performance concerns, we don't
> enable xattrs by default either.

Actually generic xattr is much worse, since there's no caching for
them currently, as opposed to posix acls, which are cached both when
positive and negative.

If we enable ACL by default in case xattrs are enabled, we should be
safe, I think.  Having an option to disable acls still makes sense,
but it's an optional plus.

Thanks,
Miklos

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

[Miklos Szeredi]

On Fri, Feb 19, 2021 at 3:34 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Fri, Feb 19, 2021 at 11:50:54AM +0000, Luis Henriques wrote:
> > Vivek Goyal <vgoyal@redhat.com> writes:
> >
> > > Hi,
> > >
> > > This is V2 of the patches. Changes since v1 are.
> > >
> > > - Rebased on top of latest master.
> > > - Took care of Miklos's comments to block acl xattrs if user
> > >   explicitly disabled posix acl.
> > >
> > > Luis Henriques reported that fstest generic/099 fails with virtiofs.
> > > Little debugging showed that we don't enable acl support. So this
> > > patch series provides option to enable/disable posix acl support. By
> > > default it is disabled.
> > >
> > > I have run blogbench and pjdfstests with posix acl enabled and
> > > things work fine.
> > >
> > > Luis, can you please apply these patches, and run virtiofsd with
> > > "-o posix_acl" and see if it fixes the failure you are seeing. I
> > > ran the steps you provided manually and it fixes the issue for
> > > me.
> >
> > Sorry for the delay.  I've finally tested these patches and they indeed
> > fix the problem I reported.  My only question about this fix is why is
> > this option not enabled by default, since this is the documented behavior
> > in acl(5) and umask(2)?  In fact, why is this an option at all?
>
> You mean why to not enable acl by default?
>
> I am concerned about performance drop this can lead to because extra
> GETXATTR(system.posix_acl_*) messages which will trigger if acls are enabled.
> And not all users might require these. That's why I preferred to not enable
> acl by default. Those who need it can enable it explicitly.
>
> Another example is xattr support. Due to performance concerns, we don't
> enable xattrs by default either.

Actually generic xattr is much worse, since there's no caching for
them currently, as opposed to posix acls, which are cached both when
positive and negative.

If we enable ACL by default in case xattrs are enabled, we should be
safe, I think.  Having an option to disable acls still makes sense,
but it's an optional plus.

Thanks,
Miklos

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

[Luis Henriques]

Miklos Szeredi <miklos@szeredi.hu> writes:

> On Fri, Feb 19, 2021 at 3:34 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>>
>> On Fri, Feb 19, 2021 at 11:50:54AM +0000, Luis Henriques wrote:
>> > Vivek Goyal <vgoyal@redhat.com> writes:
>> >
>> > > Hi,
>> > >
>> > > This is V2 of the patches. Changes since v1 are.
>> > >
>> > > - Rebased on top of latest master.
>> > > - Took care of Miklos's comments to block acl xattrs if user
>> > >   explicitly disabled posix acl.
>> > >
>> > > Luis Henriques reported that fstest generic/099 fails with virtiofs.
>> > > Little debugging showed that we don't enable acl support. So this
>> > > patch series provides option to enable/disable posix acl support. By
>> > > default it is disabled.
>> > >
>> > > I have run blogbench and pjdfstests with posix acl enabled and
>> > > things work fine.
>> > >
>> > > Luis, can you please apply these patches, and run virtiofsd with
>> > > "-o posix_acl" and see if it fixes the failure you are seeing. I
>> > > ran the steps you provided manually and it fixes the issue for
>> > > me.
>> >
>> > Sorry for the delay.  I've finally tested these patches and they indeed
>> > fix the problem I reported.  My only question about this fix is why is
>> > this option not enabled by default, since this is the documented behavior
>> > in acl(5) and umask(2)?  In fact, why is this an option at all?
>>
>> You mean why to not enable acl by default?
>>
>> I am concerned about performance drop this can lead to because extra
>> GETXATTR(system.posix_acl_*) messages which will trigger if acls are enabled.
>> And not all users might require these. That's why I preferred to not enable
>> acl by default. Those who need it can enable it explicitly.
>>
>> Another example is xattr support. Due to performance concerns, we don't
>> enable xattrs by default either.
>
> Actually generic xattr is much worse, since there's no caching for
> them currently, as opposed to posix acls, which are cached both when
> positive and negative.
>
> If we enable ACL by default in case xattrs are enabled, we should be
> safe, I think.  Having an option to disable acls still makes sense,
> but it's an optional plus.

Great, thanks for clarifying that the reason for having these options is
really for performance.

Anyway, thanks a lot for looking at this and fixing it.

Cheers,
-- 
Luis

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

[Luis Henriques]

Miklos Szeredi <miklos@szeredi.hu> writes:

> On Fri, Feb 19, 2021 at 3:34 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>>
>> On Fri, Feb 19, 2021 at 11:50:54AM +0000, Luis Henriques wrote:
>> > Vivek Goyal <vgoyal@redhat.com> writes:
>> >
>> > > Hi,
>> > >
>> > > This is V2 of the patches. Changes since v1 are.
>> > >
>> > > - Rebased on top of latest master.
>> > > - Took care of Miklos's comments to block acl xattrs if user
>> > >   explicitly disabled posix acl.
>> > >
>> > > Luis Henriques reported that fstest generic/099 fails with virtiofs.
>> > > Little debugging showed that we don't enable acl support. So this
>> > > patch series provides option to enable/disable posix acl support. By
>> > > default it is disabled.
>> > >
>> > > I have run blogbench and pjdfstests with posix acl enabled and
>> > > things work fine.
>> > >
>> > > Luis, can you please apply these patches, and run virtiofsd with
>> > > "-o posix_acl" and see if it fixes the failure you are seeing. I
>> > > ran the steps you provided manually and it fixes the issue for
>> > > me.
>> >
>> > Sorry for the delay.  I've finally tested these patches and they indeed
>> > fix the problem I reported.  My only question about this fix is why is
>> > this option not enabled by default, since this is the documented behavior
>> > in acl(5) and umask(2)?  In fact, why is this an option at all?
>>
>> You mean why to not enable acl by default?
>>
>> I am concerned about performance drop this can lead to because extra
>> GETXATTR(system.posix_acl_*) messages which will trigger if acls are enabled.
>> And not all users might require these. That's why I preferred to not enable
>> acl by default. Those who need it can enable it explicitly.
>>
>> Another example is xattr support. Due to performance concerns, we don't
>> enable xattrs by default either.
>
> Actually generic xattr is much worse, since there's no caching for
> them currently, as opposed to posix acls, which are cached both when
> positive and negative.
>
> If we enable ACL by default in case xattrs are enabled, we should be
> safe, I think.  Having an option to disable acls still makes sense,
> but it's an optional plus.

Great, thanks for clarifying that the reason for having these options is
really for performance.

Anyway, thanks a lot for looking at this and fixing it.

Cheers,
-- 
Luis

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

[Vivek Goyal]

On Fri, Feb 19, 2021 at 04:55:06PM +0100, Miklos Szeredi wrote:
> On Fri, Feb 19, 2021 at 3:34 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Fri, Feb 19, 2021 at 11:50:54AM +0000, Luis Henriques wrote:
> > > Vivek Goyal <vgoyal@redhat.com> writes:
> > >
> > > > Hi,
> > > >
> > > > This is V2 of the patches. Changes since v1 are.
> > > >
> > > > - Rebased on top of latest master.
> > > > - Took care of Miklos's comments to block acl xattrs if user
> > > >   explicitly disabled posix acl.
> > > >
> > > > Luis Henriques reported that fstest generic/099 fails with virtiofs.
> > > > Little debugging showed that we don't enable acl support. So this
> > > > patch series provides option to enable/disable posix acl support. By
> > > > default it is disabled.
> > > >
> > > > I have run blogbench and pjdfstests with posix acl enabled and
> > > > things work fine.
> > > >
> > > > Luis, can you please apply these patches, and run virtiofsd with
> > > > "-o posix_acl" and see if it fixes the failure you are seeing. I
> > > > ran the steps you provided manually and it fixes the issue for
> > > > me.
> > >
> > > Sorry for the delay.  I've finally tested these patches and they indeed
> > > fix the problem I reported.  My only question about this fix is why is
> > > this option not enabled by default, since this is the documented behavior
> > > in acl(5) and umask(2)?  In fact, why is this an option at all?
> >
> > You mean why to not enable acl by default?
> >
> > I am concerned about performance drop this can lead to because extra
> > GETXATTR(system.posix_acl_*) messages which will trigger if acls are enabled.
> > And not all users might require these. That's why I preferred to not enable
> > acl by default. Those who need it can enable it explicitly.
> >
> > Another example is xattr support. Due to performance concerns, we don't
> > enable xattrs by default either.
> 
> Actually generic xattr is much worse, since there's no caching for
> them currently, as opposed to posix acls, which are cached both when
> positive and negative.
> 
> If we enable ACL by default in case xattrs are enabled, we should be
> safe, I think.

Hi Miklos,

Ok, this sounds reasonable.  I am running some quick tests and if I don't
notice any serious performance regression, I will respin my patch.

> Having an option to disable acls still makes sense,
> but it's an optional plus.

Agreed. If there are no serious negative performance issues with enabling
ACL, then an option to disable is an optional plus.

May be I will drop this for now and add this when somebody needs an
option to disable ACL.

Thanks
Vivek

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

[Vivek Goyal]

On Fri, Feb 19, 2021 at 04:55:06PM +0100, Miklos Szeredi wrote:
> On Fri, Feb 19, 2021 at 3:34 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Fri, Feb 19, 2021 at 11:50:54AM +0000, Luis Henriques wrote:
> > > Vivek Goyal <vgoyal@redhat.com> writes:
> > >
> > > > Hi,
> > > >
> > > > This is V2 of the patches. Changes since v1 are.
> > > >
> > > > - Rebased on top of latest master.
> > > > - Took care of Miklos's comments to block acl xattrs if user
> > > >   explicitly disabled posix acl.
> > > >
> > > > Luis Henriques reported that fstest generic/099 fails with virtiofs.
> > > > Little debugging showed that we don't enable acl support. So this
> > > > patch series provides option to enable/disable posix acl support. By
> > > > default it is disabled.
> > > >
> > > > I have run blogbench and pjdfstests with posix acl enabled and
> > > > things work fine.
> > > >
> > > > Luis, can you please apply these patches, and run virtiofsd with
> > > > "-o posix_acl" and see if it fixes the failure you are seeing. I
> > > > ran the steps you provided manually and it fixes the issue for
> > > > me.
> > >
> > > Sorry for the delay.  I've finally tested these patches and they indeed
> > > fix the problem I reported.  My only question about this fix is why is
> > > this option not enabled by default, since this is the documented behavior
> > > in acl(5) and umask(2)?  In fact, why is this an option at all?
> >
> > You mean why to not enable acl by default?
> >
> > I am concerned about performance drop this can lead to because extra
> > GETXATTR(system.posix_acl_*) messages which will trigger if acls are enabled.
> > And not all users might require these. That's why I preferred to not enable
> > acl by default. Those who need it can enable it explicitly.
> >
> > Another example is xattr support. Due to performance concerns, we don't
> > enable xattrs by default either.
> 
> Actually generic xattr is much worse, since there's no caching for
> them currently, as opposed to posix acls, which are cached both when
> positive and negative.
> 
> If we enable ACL by default in case xattrs are enabled, we should be
> safe, I think.

Hi Miklos,

Ok, this sounds reasonable.  I am running some quick tests and if I don't
notice any serious performance regression, I will respin my patch.

> Having an option to disable acls still makes sense,
> but it's an optional plus.

Agreed. If there are no serious negative performance issues with enabling
ACL, then an option to disable is an optional plus.

May be I will drop this for now and add this when somebody needs an
option to disable ACL.

Thanks
Vivek

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

[Luis Henriques]

On Fri, Feb 19, 2021 at 11:50:54AM +0000, Luis Henriques wrote:
> Vivek Goyal <vgoyal@redhat.com> writes:
> 
> > Hi,
> >
> > This is V2 of the patches. Changes since v1 are.
> >
> > - Rebased on top of latest master.
> > - Took care of Miklos's comments to block acl xattrs if user
> >   explicitly disabled posix acl.
> >
> > Luis Henriques reported that fstest generic/099 fails with virtiofs.
> > Little debugging showed that we don't enable acl support. So this
> > patch series provides option to enable/disable posix acl support. By
> > default it is disabled.
> >
> > I have run blogbench and pjdfstests with posix acl enabled and
> > things work fine.
> >
> > Luis, can you please apply these patches, and run virtiofsd with
> > "-o posix_acl" and see if it fixes the failure you are seeing. I
> > ran the steps you provided manually and it fixes the issue for
> > me.
> 
> Sorry for the delay.  I've finally tested these patches and they indeed
> fix the problem I reported.  My only question about this fix is why is
> this option not enabled by default, since this is the documented behavior
> in acl(5) and umask(2)?  In fact, why is this an option at all? 

Ah!  An obvious thing that's missing: change tools/virtiofsd/helper.c to
include the new option (and also the manpage) ;-)

Cheers,
--
Luís

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

[Luis Henriques]

On Fri, Feb 19, 2021 at 11:50:54AM +0000, Luis Henriques wrote:
> Vivek Goyal <vgoyal@redhat.com> writes:
> 
> > Hi,
> >
> > This is V2 of the patches. Changes since v1 are.
> >
> > - Rebased on top of latest master.
> > - Took care of Miklos's comments to block acl xattrs if user
> >   explicitly disabled posix acl.
> >
> > Luis Henriques reported that fstest generic/099 fails with virtiofs.
> > Little debugging showed that we don't enable acl support. So this
> > patch series provides option to enable/disable posix acl support. By
> > default it is disabled.
> >
> > I have run blogbench and pjdfstests with posix acl enabled and
> > things work fine.
> >
> > Luis, can you please apply these patches, and run virtiofsd with
> > "-o posix_acl" and see if it fixes the failure you are seeing. I
> > ran the steps you provided manually and it fixes the issue for
> > me.
> 
> Sorry for the delay.  I've finally tested these patches and they indeed
> fix the problem I reported.  My only question about this fix is why is
> this option not enabled by default, since this is the documented behavior
> in acl(5) and umask(2)?  In fact, why is this an option at all? 

Ah!  An obvious thing that's missing: change tools/virtiofsd/helper.c to
include the new option (and also the manpage) ;-)

Cheers,
--
Luís