Re: [PATCH v2 18/21] backing-inode: add backing_inode_setattr()

On Sat, May 16, 2026 at 2:53 AM Joanne Koong <[email protected]> wrote:
>
> Move logic in ovl_setattr() to a generic backing_inode_setattr() function
> in fs/backing-inode.c, which other filesystems that use backing inodes
> (eg fuse passthrough) will use.
>
> Signed-off-by: Joanne Koong <[email protected]>

Wow this code brings back memories that I tried hard to forget :)
I hope that fuse passthrough can reap the benefits of all the bugs
and fixes in ovl_setattr()...

Reviewed-by: Amir Goldstein <[email protected]>

> ---
>  fs/backing-inode.c            | 54 ++++++++++++++++++++++++++++
>  fs/overlayfs/inode.c          | 66 ++++++++++-------------------------
>  include/linux/backing-inode.h |  4 +++
>  3 files changed, 76 insertions(+), 48 deletions(-)
>
> diff --git a/fs/backing-inode.c b/fs/backing-inode.c
> index 474770a1fa9d..e72f278789d8 100644
> --- a/fs/backing-inode.c
> +++ b/fs/backing-inode.c
> @@ -41,3 +41,57 @@ void backing_inode_copyattr(struct inode *inode,
>  }
>  EXPORT_SYMBOL_GPL(backing_inode_copyattr);
>
> +int backing_inode_setattr(struct dentry *dentry,
> +                         const struct path *backing_path,
> +                         struct iattr *attr, const struct cred *cred)
> +{
> +       struct dentry *backing_dentry = backing_path->dentry;
> +       struct inode *winode = NULL;
> +       int err;
> +
> +       if (attr->ia_valid & ATTR_SIZE) {
> +               winode = d_inode(backing_dentry);
> +               err = get_write_access(winode);
> +               if (err)
> +                       return err;
> +       }
> +
> +       /*
> +        * Clear ATTR_MODE to avoid BUG_ON in notify_change(), which does not
> +        * allow ATTR_MODE together with ATTR_KILL_SUID/SGID (the first
> +        * notify_change() converted ATTR_KILL_SUID/SGID into ATTR_MODE using
> +        * @dentry's inode mode).
> +        *
> +        * This also ensures notify_change() recomputes the mode from the
> +        * backing inode's current mode, instead of a potentially stale value
> +        * from @dentry's inode.
> +        */
> +       if (attr->ia_valid & (ATTR_KILL_SUID|ATTR_KILL_SGID))
> +               attr->ia_valid &= ~ATTR_MODE;
> +
> +       /*
> +        * The filesystem's file is not meaningful to the backing filesystem.
> +        * Clear ATTR_FILE so the backing filesystem does not try to use it.
> +        */
> +       attr->ia_valid &= ~ATTR_FILE;
> +
> +       err = mnt_want_write(backing_path->mnt);
> +       if (err)
> +               goto out_put_write;
> +
> +       inode_lock(backing_dentry->d_inode);
> +       scoped_with_creds(cred)
> +               err = notify_change(mnt_idmap(backing_path->mnt),
> +                                   backing_dentry, attr, NULL);
> +       if (!err)
> +               backing_inode_copyattr(dentry->d_inode, backing_path);
> +       inode_unlock(backing_dentry->d_inode);
> +       mnt_drop_write(backing_path->mnt);
> +
> +out_put_write:
> +       if (winode)
> +               put_write_access(winode);
> +
> +       return err;
> +}
> +EXPORT_SYMBOL_GPL(backing_inode_setattr);
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 00c69707bda9..f1cd67e6e9c8 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -15,16 +15,17 @@
>  #include <linux/namei.h>
>  #include <linux/posix_acl.h>
>  #include <linux/posix_acl_xattr.h>
> +#include <linux/backing-inode.h>
>  #include "overlayfs.h"
>
>
>  int ovl_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>                 struct iattr *attr)
>  {
> +       struct path backing_path;
>         int err;
>         struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
>         bool full_copy_up = false;
> -       struct dentry *upperdentry;
>
>         err = setattr_prepare(&nop_mnt_idmap, dentry, attr);
>         if (err)
> @@ -39,57 +40,26 @@ int ovl_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>                 err = ovl_copy_up(dentry);
>         else
>                 err = ovl_copy_up_with_data(dentry);
> -       if (!err) {
> -               struct inode *winode = NULL;
> -
> -               upperdentry = ovl_dentry_upper(dentry);
> -
> -               if (attr->ia_valid & ATTR_SIZE) {
> -                       winode = d_inode(upperdentry);
> -                       err = get_write_access(winode);
> -                       if (err)
> -                               goto out;
> -               }
> -
> -               if (attr->ia_valid & (ATTR_KILL_SUID|ATTR_KILL_SGID))
> -                       attr->ia_valid &= ~ATTR_MODE;
>
> -               /*
> -                * We might have to translate ovl file into real file object
> -                * once use cases emerge.  For now, simply don't let underlying
> -                * filesystem rely on attr->ia_file
> -                */
> -               attr->ia_valid &= ~ATTR_FILE;
> -
> -               /*
> -                * If open(O_TRUNC) is done, VFS calls ->setattr with ATTR_OPEN
> -                * set.  Overlayfs does not pass O_TRUNC flag to underlying
> -                * filesystem during open -> do not pass ATTR_OPEN.  This
> -                * disables optimization in fuse which assumes open(O_TRUNC)
> -                * already set file size to 0.  But we never passed O_TRUNC to
> -                * fuse.  So by clearing ATTR_OPEN, fuse will be forced to send
> -                * setattr request to server.
> -                */
> -               attr->ia_valid &= ~ATTR_OPEN;
> +       if (err)
> +               return err;
>
> -               err = ovl_want_write(dentry);
> -               if (err)
> -                       goto out_put_write;
> +       /*
> +        * If open(O_TRUNC) is done, VFS calls ->setattr with ATTR_OPEN
> +        * set.  Overlayfs does not pass O_TRUNC flag to underlying
> +        * filesystem during open -> do not pass ATTR_OPEN.  This
> +        * disables optimization in fuse which assumes open(O_TRUNC)
> +        * already set file size to 0.  But we never passed O_TRUNC to
> +        * fuse.  So by clearing ATTR_OPEN, fuse will be forced to send
> +        * setattr request to server.
> +        */
> +       attr->ia_valid &= ~ATTR_OPEN;
>
> -               inode_lock(upperdentry->d_inode);
> -               with_ovl_creds(dentry->d_sb)
> -                       err = ovl_do_notify_change(ofs, upperdentry, attr);
> -               if (!err)
> -                       ovl_copyattr(dentry->d_inode);
> -               inode_unlock(upperdentry->d_inode);
> -               ovl_drop_write(dentry);
> +       backing_path.dentry = ovl_dentry_upper(dentry);
> +       backing_path.mnt = ovl_upper_mnt(ofs);
>
> -out_put_write:
> -               if (winode)
> -                       put_write_access(winode);
> -       }
> -out:
> -       return err;
> +       return backing_inode_setattr(dentry, &backing_path, attr,
> +                                    ovl_creds(dentry->d_sb));
>  }
>
>  static void ovl_map_dev_ino(struct dentry *dentry, struct kstat *stat, int fsid)
> diff --git a/include/linux/backing-inode.h b/include/linux/backing-inode.h
> index 6b43cba9fabd..fd02c87e0f99 100644
> --- a/include/linux/backing-inode.h
> +++ b/include/linux/backing-inode.h
> @@ -11,4 +11,8 @@
>  void backing_inode_copyattr(struct inode *inode,
>                             const struct path *backing_path);
>
> +int backing_inode_setattr(struct dentry *dentry,
> +                         const struct path *backing_path,
> +                         struct iattr *attr, const struct cred *cred);
> +
>  #endif /* _LINUX_BACKING_INODE_H */
> --
> 2.52.0
>