linux-ext4 - Re: [PATCH 4/5] fuse: update file mode when updating acls

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJnrk1apJbki7aZq2tNnnBcbkGKUmWDfmXVBD5YaMKUH2Fd-FA@mail.gmail.com>
Date: Fri, 7 Nov 2025 12:29:04 -0800
From: Joanne Koong <joannelkoong@...il.com>
To: "Darrick J. Wong" <djwong@...nel.org>
Cc: miklos@...redi.hu, bernd@...ernd.com, neal@...pa.dev, 
	linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 4/5] fuse: update file mode when updating acls

On Tue, Oct 28, 2025 at 5:43 PM Darrick J. Wong <djwong@...nel.org> wrote:
>
> From: Darrick J. Wong <djwong@...nel.org>
>
> If someone sets ACLs on a file that can be expressed fully as Unix DAC
> mode bits, most local filesystems will then update the mode bits and
> drop the ACL xattr to reduce inefficiency in the file access paths.
> Let's do that too.  Note that means that we can setacl and end up with
> no ACL xattrs, so we also need to tolerate ENODATA returns from
> fuse_removexattr.
>
> Note that here we define a "local" fuse filesystem as one that uses
> fuseblk mode; we'll shortly add fuse servers that use iomap for the file
> IO path to that list.
>
> Signed-off-by: "Darrick J. Wong" <djwong@...nel.org>
> ---
>  fs/fuse/fuse_i.h |    2 +-
>  fs/fuse/acl.c    |   43 ++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 43 insertions(+), 2 deletions(-)
>
>
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 8c47d103c8ffa6..d550937770e16e 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -1050,7 +1050,7 @@ static inline struct fuse_mount *get_fuse_mount(struct inode *inode)
>         return get_fuse_mount_super(inode->i_sb);
>  }
>
> -static inline struct fuse_conn *get_fuse_conn(struct inode *inode)
> +static inline struct fuse_conn *get_fuse_conn(const struct inode *inode)
>  {
>         return get_fuse_mount_super(inode->i_sb)->fc;
>  }
> diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c
> index 8f484b105f13ab..72bb4c94079b7b 100644
> --- a/fs/fuse/acl.c
> +++ b/fs/fuse/acl.c
> @@ -11,6 +11,18 @@
>  #include <linux/posix_acl.h>
>  #include <linux/posix_acl_xattr.h>
>
> +/*
> + * If this fuse server behaves like a local filesystem, we can implement the
> + * kernel's optimizations for ACLs for local filesystems instead of passing
> + * the ACL requests straight through to another server.
> + */
> +static inline bool fuse_inode_has_local_acls(const struct inode *inode)
> +{
> +       const struct fuse_conn *fc = get_fuse_conn(inode);
> +
> +       return fc->posix_acl && fuse_inode_is_exclusive(inode);
> +}
> +
>  static struct posix_acl *__fuse_get_acl(struct fuse_conn *fc,
>                                         struct inode *inode, int type, bool rcu)
>  {
> @@ -98,6 +110,7 @@ int fuse_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
>         struct inode *inode = d_inode(dentry);
>         struct fuse_conn *fc = get_fuse_conn(inode);
>         const char *name;
> +       umode_t mode = inode->i_mode;
>         int ret;
>
>         if (fuse_is_bad(inode))
> @@ -113,6 +126,18 @@ int fuse_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
>         else
>                 return -EINVAL;
>
> +       /*
> +        * If the ACL can be represented entirely with changes to the mode
> +        * bits, then most filesystems will update the mode bits and delete
> +        * the ACL xattr.
> +        */
> +       if (acl && type == ACL_TYPE_ACCESS &&
> +           fuse_inode_has_local_acls(inode)) {
> +               ret = posix_acl_update_mode(idmap, inode, &mode, &acl);
> +               if (ret)
> +                       return ret;
> +       }

nit: this could be inside the if (acl) block below.

I'm not too familiar with ACLs so i'll abstain from adding my
Reviewed-by to this.

Thanks,
Joanne

> +
>         if (acl) {
>                 unsigned int extra_flags = 0;
>                 /*
> @@ -143,7 +168,7 @@ int fuse_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
>                  * through POSIX ACLs. Such daemons don't expect setgid bits to
>                  * be stripped.
>                  */
> -               if (fc->posix_acl &&
> +               if (fc->posix_acl && mode == inode->i_mode &&
>                     !in_group_or_capable(idmap, inode,
>                                          i_gid_into_vfsgid(idmap, inode)))
>                         extra_flags |= FUSE_SETXATTR_ACL_KILL_SGID;
> @@ -152,6 +177,22 @@ int fuse_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
>                 kfree(value);
>         } else {
>                 ret = fuse_removexattr(inode, name);
> +               /* If the acl didn't exist to start with that's fine. */
> +               if (ret == -ENODATA)
> +                       ret = 0;
> +       }
> +
> +       /* If we scheduled a mode update above, push that to userspace now. */
> +       if (!ret) {
> +               struct iattr attr = { };
> +
> +               if (mode != inode->i_mode) {
> +                       attr.ia_valid |= ATTR_MODE;
> +                       attr.ia_mode = mode;
> +               }
> +
> +               if (attr.ia_valid)
> +                       ret = fuse_do_setattr(idmap, dentry, &attr, NULL);
>         }
>
>         if (fc->posix_acl) {
>