Re: [PATCH 3/4] fuse: update file mode when updating acls

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Apr 30, 2026 at 02:48:29PM +0100, Joanne Koong wrote:
> On Wed, Apr 29, 2026 at 3:22 PM Darrick J. Wong <djwong@kernel.org> wrote:
> >
> > From: Darrick J. Wong <djwong@kernel.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@kernel.org>
> > ---
> >  fs/fuse/fuse_i.h |    2 +-
> >  fs/fuse/acl.c    |   56 +++++++++++++++++++++++++++++++++++++++++++++++-------
> >  2 files changed, 50 insertions(+), 8 deletions(-)
> >
> >
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index 4c135a7edf54ac..0bcfb42592895c 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 cbde6ac1add35a..bee8a9a734f50a 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.
> 
> nit: "to the server" instead of "to another server"? another server
> sounds like there's 2 servers
> 
> > + */
> > +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,8 @@ 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;
> > +       const bool local_acls = fuse_inode_has_local_acls(inode);
> >         int ret;
> >
> >         if (fuse_is_bad(inode))
> > @@ -113,14 +127,25 @@ 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 && local_acls) {
> > +               ret = posix_acl_update_mode(idmap, inode, &mode, &acl);
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> >         if (acl) {
> >                 unsigned int extra_flags = 0;
> >                 /*
> > -                * Fuse userspace is responsible for updating access
> > -                * permissions in the inode, if needed. fuse_setxattr
> > -                * invalidates the inode attributes, which will force
> > -                * them to be refreshed the next time they are used,
> > -                * and it also updates i_ctime.
> > +                * For non-local filesystems, fuse userspace is responsible for
> > +                * updating access permissions in the inode, if needed.
> > +                * fuse_setxattr invalidates the inode attributes, which will
> > +                * force them to be refreshed the next time they are used, and
> > +                * it also updates i_ctime.
> >                  */
> >                 size_t size;
> >                 void *value;
> > @@ -137,9 +162,10 @@ int fuse_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
> >                 /*
> >                  * Fuse daemons without FUSE_POSIX_ACL never changed the passed
> >                  * through POSIX ACLs. Such daemons don't expect setgid bits to
> > -                * be stripped.
> > +                * be stripped, unless they've explicitly told the kernel to
> > +                * take care of that.
> 
> nit: imo this would be clearer as its own sentence, eg "For local
> filesystems, the kernel already handled sgid stripping in
> posix_acl_update_mode() above".
> 
> >                  */
> > -               if (fc->posix_acl &&
> > +               if (fc->posix_acl && !local_acls &&
> >                     !in_group_or_capable(idmap, inode,
> >                                          i_gid_into_vfsgid(idmap, inode)))
> >                         extra_flags |= FUSE_SETXATTR_ACL_KILL_SGID;
> > @@ -148,6 +174,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);
> 
> maybe something like this is clearer?
> 
>     if (!ret && mode != inode->i_mode) {
>          struct iattr attr = {
>                 .ia_valid |= ATTR_MODE,
>                 .ia_mode = mode,
>           };
>          ret = fuse_do_setattr(idmap, dentry, &attr, NULL);
>     }

I know, you said that last time ;)
https://lore.kernel.org/linux-fsdevel/CAJnrk1YL3KnON-WtNjNi+2GZ+6rYvnVUnYwCk5efv0o41XkxcA@mail.gmail.com/

and my reply is the same:
https://lore.kernel.org/linux-fsdevel/20260325222308.GH6202@frogsfrogsfrogs/#t

...which is to say, does anyone think that's worth the churn?

> 
> Overall, this lgtm though.
> 
> Reviewed-by: Joanne Koong <joannelkoong@gmail.com>

Thanks!

--D

> Thanks,
> Joanne
> 
> >         }
> >
> >         if (fc->posix_acl) {
> >
>