Re: [PATCH v3 4/4] ovl: consistent behavior for immutable/append-only inodes

[Miklos Szeredi <miklos@szeredi.hu>]

On Sat, 19 Jun 2021 at 11:26, Amir Goldstein <amir73il@gmail.com> wrote:
>
> When a lower file has immutable/append-only fileattr flags, the behavior
> of overlayfs post copy up is inconsistent.
>
> Immediattely after copy up, ovl inode still has the S_IMMUTABLE/S_APPEND
> inode flags copied from lower inode, so vfs code still treats the ovl
> inode as immutable/append-only.  After ovl inode evict or mount cycle,
> the ovl inode does not have these inode flags anymore.
>
> We cannot copy up the immutable and append-only fileattr flags, because
> immutable/append-only inodes cannot be linked and because overlayfs will
> not be able to set overlay.* xattr on the upper inodes.
>
> Instead, if any of the fileattr flags of interest exist on the lower
> inode, we store them in overlay.protected xattr on the upper inode and we
> we read the flags from xattr on lookup and on fileattr_get().
>
> This gives consistent behavior post copy up regardless of inode eviction
> from cache.
>
> When user sets new fileattr flags, we update or remove the
> overlay.protected xattr.
>
> Storing immutable/append-only fileattr flags in an xattr instead of upper
> fileattr also solves other non-standard behavior issues - overlayfs can
> now copy up children of "ovl-immutable" directories and lower aliases of
> "ovl-immutable" hardlinks.
>
> Reported-by: Chengguang Xu <cgxu519@mykernel.net>
> Link: https://lore.kernel.org/linux-unionfs/20201226104618.239739-1-cgxu519@mykernel.net/
> Link: https://lore.kernel.org/linux-unionfs/20210210190334.1212210-5-amir73il@gmail.com/
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Some notes and questions below.  I can take care of fixing these up.

> ---
>  fs/overlayfs/copy_up.c   |  17 +++++-
>  fs/overlayfs/inode.c     |  42 +++++++++++++-
>  fs/overlayfs/overlayfs.h |  28 +++++++++-
>  fs/overlayfs/util.c      | 115 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 195 insertions(+), 7 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index a06b423ca5d1..fc9ffcf32d0c 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -131,7 +131,8 @@ int ovl_copy_xattr(struct super_block *sb, struct dentry *old,
>         return error;
>  }
>
> -static int ovl_copy_fileattr(struct path *old, struct path *new)
> +static int ovl_copy_fileattr(struct inode *inode, struct path *old,
> +                            struct path *new)
>  {
>         struct fileattr oldfa = { .flags_valid = true };
>         struct fileattr newfa = { .flags_valid = true };
> @@ -145,6 +146,18 @@ static int ovl_copy_fileattr(struct path *old, struct path *new)
>         if (err)
>                 return err;
>
> +       /*
> +        * We cannot set immutable and append-only flags on upper inode,
> +        * because we would not be able to link upper inode to upper dir
> +        * not set overlay private xattr on upper inode.
> +        * Store these flags in overlay.protected xattr instead.
> +        */
> +       if (oldfa.flags & OVL_PROT_FS_FLAGS_MASK) {
> +               err = ovl_set_protected(inode, new->dentry, &oldfa);
> +               if (err)
> +                       return err;
> +       }
> +
>         BUILD_BUG_ON(OVL_COPY_FS_FLAGS_MASK & ~FS_COMMON_FL);
>         newfa.flags &= ~OVL_COPY_FS_FLAGS_MASK;
>         newfa.flags |= (oldfa.flags & OVL_COPY_FS_FLAGS_MASK);
> @@ -550,7 +563,7 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
>                  * Copy the fileattr inode flags that are the source of already
>                  * copied i_flags (best effort).
>                  */
> -               ovl_copy_fileattr(&c->lowerpath, &upperpath);
> +               ovl_copy_fileattr(inode, &c->lowerpath, &upperpath);
>         }
>
>         /*
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index aec353a2dc80..9c621ed17a61 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -162,7 +162,8 @@ int ovl_getattr(struct user_namespace *mnt_userns, const struct path *path,
>         enum ovl_path_type type;
>         struct path realpath;
>         const struct cred *old_cred;
> -       bool is_dir = S_ISDIR(dentry->d_inode->i_mode);
> +       struct inode *inode = d_inode(dentry);
> +       bool is_dir = S_ISDIR(inode->i_mode);
>         int fsid = 0;
>         int err;
>         bool metacopy_blocks = false;
> @@ -175,6 +176,10 @@ int ovl_getattr(struct user_namespace *mnt_userns, const struct path *path,
>         if (err)
>                 goto out;
>
> +       /* Report the effective immutable/append-only STATX flags */
> +       if (ovl_test_flag(OVL_PROTECTED, inode))
> +               generic_fill_statx_attr(inode, stat);

Assuming i_flags is correct this doesn't need to be conditional on
OVL_PROTECTED, right?


> +
>         /*
>          * For non-dir or same fs, we use st_ino of the copy up origin.
>          * This guaranties constant st_dev/st_ino across copy up.
> @@ -556,15 +561,40 @@ int ovl_fileattr_set(struct user_namespace *mnt_userns,
>                 ovl_path_real(dentry, &upperpath);
>
>                 old_cred = ovl_override_creds(inode->i_sb);
> -               err = ovl_real_fileattr(&upperpath, fa, true);
> +               /*
> +                * Store immutable/append-only flags in xattr and clear them
> +                * in upper fileattr (in case they were set by older kernel)
> +                * so children of "ovl-immutable" directories lower aliases of
> +                * "ovl-immutable" hardlinks could be copied up.
> +                * Clear xattr when flags are cleared.
> +                */
> +               err = ovl_set_protected(inode, upperpath.dentry, fa);
> +               if (!err)
> +                       err = ovl_real_fileattr(&upperpath, fa, true);
>                 revert_creds(old_cred);
> -               ovl_copyflags(ovl_inode_real(inode), inode);
> +               ovl_merge_prot_flags(ovl_inode_real(inode), inode);
>         }
>         ovl_drop_write(dentry);
>  out:
>         return err;
>  }
>
> +/* Convert inode protection flags to fileattr flags */
> +static void ovl_fileattr_prot_flags(struct inode *inode, struct fileattr *fa)
> +{
> +       BUILD_BUG_ON(OVL_PROT_FS_FLAGS_MASK & ~FS_COMMON_FL);
> +       BUILD_BUG_ON(OVL_PROT_FSX_FLAGS_MASK & ~FS_XFLAG_COMMON);
> +
> +       if (inode->i_flags & S_APPEND) {
> +               fa->flags |= FS_APPEND_FL;
> +               fa->fsx_xflags |= FS_XFLAG_APPEND;
> +       }
> +       if (inode->i_flags & S_IMMUTABLE) {
> +               fa->flags |= FS_IMMUTABLE_FL;
> +               fa->fsx_xflags |= FS_XFLAG_IMMUTABLE;
> +       }
> +}
> +
>  int ovl_fileattr_get(struct dentry *dentry, struct fileattr *fa)
>  {
>         struct inode *inode = d_inode(dentry);
> @@ -576,6 +606,8 @@ int ovl_fileattr_get(struct dentry *dentry, struct fileattr *fa)
>
>         old_cred = ovl_override_creds(inode->i_sb);
>         err = ovl_real_fileattr(&realpath, fa, false);
> +       if (!err && ovl_test_flag(OVL_PROTECTED, inode))
> +               ovl_fileattr_prot_flags(inode, fa);

Again, I don't see a reason making this conditional on OVL_PROTECTED.

>         revert_creds(old_cred);
>
>         return err;
> @@ -1128,6 +1160,10 @@ struct inode *ovl_get_inode(struct super_block *sb,
>                 }
>         }
>
> +       /* Check for immutable/append-only inode flags in xattr */
> +       if (upperdentry)
> +               ovl_check_protected(inode, upperdentry);
> +
>         if (inode->i_state & I_NEW)
>                 unlock_new_inode(inode);
>  out:
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 1e964e4e45d4..840b6e1a71ea 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -34,6 +34,7 @@ enum ovl_xattr {
>         OVL_XATTR_NLINK,
>         OVL_XATTR_UPPER,
>         OVL_XATTR_METACOPY,
> +       OVL_XATTR_PROTECTED,
>  };
>
>  enum ovl_inode_flag {
> @@ -45,6 +46,8 @@ enum ovl_inode_flag {
>         OVL_UPPERDATA,
>         /* Inode number will remain constant over copy up. */
>         OVL_CONST_INO,
> +       /* Has overlay.protected xattr */
> +       OVL_PROTECTED,
>  };
>
>  enum ovl_entry_flag {
> @@ -532,14 +535,22 @@ static inline void ovl_copyattr(struct inode *from, struct inode *to)
>
>  /* vfs inode flags copied from real to ovl inode */
>  #define OVL_COPY_I_FLAGS_MASK  (S_SYNC | S_NOATIME | S_APPEND | S_IMMUTABLE)
> +/* vfs inode flags read from overlay.protected xattr to ovl inode */
> +#define OVL_PROT_I_FLAGS_MASK  (S_APPEND | S_IMMUTABLE)
>
>  /*
>   * fileattr flags copied from lower to upper inode on copy up.
> - * We cannot copy immutable/append-only flags, because that would prevevnt
> - * linking temp inode to upper dir.
> + * We cannot copy up immutable/append-only flags, because that would prevevnt
> + * linking temp inode to upper dir, so we store them in xattr instead.
>   */
>  #define OVL_COPY_FS_FLAGS_MASK (FS_SYNC_FL | FS_NOATIME_FL)
>  #define OVL_COPY_FSX_FLAGS_MASK        (FS_XFLAG_SYNC | FS_XFLAG_NOATIME)
> +#define OVL_PROT_FS_FLAGS_MASK  (FS_APPEND_FL | FS_IMMUTABLE_FL)
> +#define OVL_PROT_FSX_FLAGS_MASK (FS_XFLAG_APPEND | FS_XFLAG_IMMUTABLE)
> +
> +bool ovl_check_protected(struct inode *inode, struct dentry *upper);
> +int ovl_set_protected(struct inode *inode, struct dentry *upper,
> +                     struct fileattr *fa);
>
>  static inline void ovl_copyflags(struct inode *from, struct inode *to)
>  {
> @@ -548,6 +559,19 @@ static inline void ovl_copyflags(struct inode *from, struct inode *to)
>         inode_set_flags(to, from->i_flags & mask, mask);
>  }
>
> +/* Merge real inode flags with inode flags read from overlay.protected xattr */
> +static inline void ovl_merge_prot_flags(struct inode *real, struct inode *inode)
> +{
> +       unsigned int flags = real->i_flags & OVL_COPY_I_FLAGS_MASK;
> +
> +       BUILD_BUG_ON(OVL_PROT_I_FLAGS_MASK & ~OVL_COPY_I_FLAGS_MASK);
> +
> +       if (ovl_test_flag(OVL_PROTECTED, inode))
> +               flags |= inode->i_flags & OVL_PROT_I_FLAGS_MASK;

And here also.

> +
> +       inode_set_flags(inode, flags, OVL_COPY_I_FLAGS_MASK);
> +}
> +
>  /* dir.c */
>  extern const struct inode_operations ovl_dir_inode_operations;
>  int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct inode *dir,
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 81b8f135445a..202377ca2ee9 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -10,6 +10,7 @@
>  #include <linux/cred.h>
>  #include <linux/xattr.h>
>  #include <linux/exportfs.h>
> +#include <linux/fileattr.h>
>  #include <linux/uuid.h>
>  #include <linux/namei.h>
>  #include <linux/ratelimit.h>
> @@ -585,6 +586,7 @@ bool ovl_check_dir_xattr(struct super_block *sb, struct dentry *dentry,
>  #define OVL_XATTR_NLINK_POSTFIX                "nlink"
>  #define OVL_XATTR_UPPER_POSTFIX                "upper"
>  #define OVL_XATTR_METACOPY_POSTFIX     "metacopy"
> +#define OVL_XATTR_PROTECTED_POSTFIX    "protected"
>
>  #define OVL_XATTR_TAB_ENTRY(x) \
>         [x] = { [false] = OVL_XATTR_TRUSTED_PREFIX x ## _POSTFIX, \
> @@ -598,6 +600,7 @@ const char *const ovl_xattr_table[][2] = {
>         OVL_XATTR_TAB_ENTRY(OVL_XATTR_NLINK),
>         OVL_XATTR_TAB_ENTRY(OVL_XATTR_UPPER),
>         OVL_XATTR_TAB_ENTRY(OVL_XATTR_METACOPY),
> +       OVL_XATTR_TAB_ENTRY(OVL_XATTR_PROTECTED),
>  };
>
>  int ovl_check_setxattr(struct ovl_fs *ofs, struct dentry *upperdentry,
> @@ -639,6 +642,118 @@ int ovl_set_impure(struct dentry *dentry, struct dentry *upperdentry)
>         return err;
>  }
>
> +
> +/*
> + * Initialize inode flags from overlay.protected xattr and upper inode flags.
> + * If upper inode has those fileattr flags set (i.e. from old kernel), we do not
> + * clear them on ovl_get_inode(), but we will clear them on next fileattr_set().
> + */
> +static bool ovl_prot_flags_from_buf(struct inode *inode, const char *buf,
> +                                   int len)
> +{
> +       u32 iflags = inode->i_flags & OVL_PROT_I_FLAGS_MASK;
> +       int n;
> +
> +       /*
> +        * We cannot clear flags that are set on real inode.
> +        * We can only set flags that are not set in inode.
> +        */
> +       for (n = 0; n < len && buf[n]; n++) {
> +               if (buf[n] == 'a')
> +                       iflags |= S_APPEND;
> +               else if (buf[n] == 'i')
> +                       iflags |= S_IMMUTABLE;
> +               else
> +                       break;
> +       }
> +
> +       inode_set_flags(inode, iflags, OVL_PROT_I_FLAGS_MASK);
> +
> +       return buf[n] == 0;
> +}
> +
> +#define OVL_PROTECTED_MAX 32 /* Reserved for future flags */
> +
> +bool ovl_check_protected(struct inode *inode, struct dentry *upper)
> +{
> +       struct ovl_fs *ofs = OVL_FS(inode->i_sb);
> +       char buf[OVL_PROTECTED_MAX+1];
> +       int res;
> +
> +       res = ovl_do_getxattr(ofs, upper, OVL_XATTR_PROTECTED, buf,
> +                             OVL_PROTECTED_MAX);
> +       if (res < 0)
> +               return false;
> +
> +       buf[res] = 0;
> +       if (res == 0 || !ovl_prot_flags_from_buf(inode, buf, res)) {
> +               pr_warn_ratelimited("incompatible overlay.protected format (%pd2, len=%d)\n",
> +                                   upper, res);
> +       }
> +
> +       ovl_set_flag(OVL_PROTECTED, inode);
> +       ovl_merge_prot_flags(d_inode(upper), inode);

ovl_prot_flags_from_buf() should have updated i_flags, so this looks
like a no-op.  Or am I missing something?

> +
> +       return true;
> +}
> +
> +/* Set inode flags and overlay.protected xattr from fileattr */
> +static int ovl_prot_flags_to_buf(struct inode *inode, char *buf,
> +                                const struct fileattr *fa)
> +{
> +       u32 iflags = 0;
> +       int n = 0;
> +
> +       if (fa->flags & FS_APPEND_FL) {
> +               buf[n++] = 'a';
> +               iflags |= S_APPEND;
> +       }
> +       if (fa->flags & FS_IMMUTABLE_FL) {
> +               buf[n++] = 'i';
> +               iflags |= S_IMMUTABLE;
> +       }
> +
> +       inode_set_flags(inode, iflags, OVL_PROT_I_FLAGS_MASK);

Looks like the wrong place to update i_flags, since the setxattr may yet fail.

> +
> +       return n;
> +}
> +
> +int ovl_set_protected(struct inode *inode, struct dentry *upper,
> +                     struct fileattr *fa)
> +{
> +       struct ovl_fs *ofs = OVL_FS(inode->i_sb);
> +       char buf[OVL_PROTECTED_MAX];
> +       int len, err = 0;
> +
> +       BUILD_BUG_ON(HWEIGHT32(OVL_PROT_FS_FLAGS_MASK) > OVL_PROTECTED_MAX);
> +       len = ovl_prot_flags_to_buf(inode, buf, fa);
> +
> +       /*
> +        * Do not allow to set protection flags when upper doesn't support
> +        * xattrs, because we do not set those fileattr flags on upper inode.
> +        * Remove xattr if it exist and all protection flags are cleared.
> +        */
> +       if (len) {
> +               err = ovl_check_setxattr(ofs, upper, OVL_XATTR_PROTECTED,
> +                                        buf, len, -EPERM);
> +       } else if (ovl_test_flag(OVL_PROTECTED, inode)) {

Last place OVL_PROTECTED is tested.   What about just translating
ENODATA/EOPNOTSUPP to success?

That way OVL_PROTECTED could be dropped altogether.

> +               err = ovl_do_removexattr(ofs, upper, OVL_XATTR_PROTECTED);
> +       }
> +       if (err)
> +               return err;
> +
> +       /* Mask out the fileattr flags that should not be set in upper inode */
> +       fa->flags &= ~OVL_PROT_FS_FLAGS_MASK;
> +       fa->fsx_xflags &= ~OVL_PROT_FSX_FLAGS_MASK;
> +
> +       if (len)
> +               ovl_set_flag(OVL_PROTECTED, inode);
> +       else
> +               ovl_clear_flag(OVL_PROTECTED, inode);
> +
> +       return 0;
> +}
> +
>  /**
>   * Caller must hold a reference to inode to prevent it from being freed while
>   * it is marked inuse.
> --
> 2.32.0
>