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: <20251108001753.GL196391@frogsfrogsfrogs>
Date: Fri, 7 Nov 2025 16:17:53 -0800
From: "Darrick J. Wong" <djwong@...nel.org>
To: Joanne Koong <joannelkoong@...il.com>
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 Fri, Nov 07, 2025 at 12:29:04PM -0800, Joanne Koong wrote:
> 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.

posix_acl_update_mode can set acl to NULL.

--D

> 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) {
> >