On Mon, Jul 12, 2021 at 6:51 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > On Mon, Jul 12, 2021 at 5:20 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote: > > > > On Sat, 19 Jun 2021 at 11:26, Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > > > > When a lower file has sync/noatime fileattr flags, the behavior of > > > overlayfs post copy up is inconsistent. > > > > > > Immediattely after copy up, ovl inode still has the S_SYNC/S_NOATIME > > > inode flags copied from lower inode, so vfs code still treats the ovl > > > inode as sync/noatime. After ovl inode evict or mount cycle, > > > the ovl inode does not have these inode flags anymore. > > > > > > To fix this inconsitency, try to copy the fileattr flags on copy up > > > if the upper fs supports the fileattr_set() method. > > > > > > This gives consistent behavior post copy up regardless of inode eviction > > > from cache. > > > > > > We cannot copy up the immutable/append-only inode flags in a similar > > > manner, because immutable/append-only inodes cannot be linked and because > > > overlayfs will not be able to set overlay.* xattr on the upper inodes. > > > > > > Those flags will be addressed by a followup patch. > > > > > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx> > > > --- > > > fs/overlayfs/copy_up.c | 49 ++++++++++++++++++++++++++++++++++------ > > > fs/overlayfs/inode.c | 36 ++++++++++++++++++----------- > > > fs/overlayfs/overlayfs.h | 14 +++++++++++- > > > 3 files changed, 78 insertions(+), 21 deletions(-) > > > > > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c > > > index 3fa68a5cc16e..a06b423ca5d1 100644 > > > --- a/fs/overlayfs/copy_up.c > > > +++ b/fs/overlayfs/copy_up.c > > > @@ -8,6 +8,7 @@ > > > #include <linux/fs.h> > > > #include <linux/slab.h> > > > #include <linux/file.h> > > > +#include <linux/fileattr.h> > > > #include <linux/splice.h> > > > #include <linux/xattr.h> > > > #include <linux/security.h> > > > @@ -130,6 +131,31 @@ int ovl_copy_xattr(struct super_block *sb, struct dentry *old, > > > return error; > > > } > > > > > > +static int ovl_copy_fileattr(struct path *old, struct path *new) > > > +{ > > > + struct fileattr oldfa = { .flags_valid = true }; > > > + struct fileattr newfa = { .flags_valid = true }; > > > + int err; > > > + > > > + err = ovl_real_fileattr(old, &oldfa, false); > > > + if (err) > > > + return err; > > > + > > > + err = ovl_real_fileattr(new, &newfa, false); > > > + 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); > > > + > > > + BUILD_BUG_ON(OVL_COPY_FSX_FLAGS_MASK & ~FS_XFLAG_COMMON); > > > + newfa.fsx_xflags &= ~OVL_COPY_FSX_FLAGS_MASK; > > > + newfa.fsx_xflags |= (oldfa.fsx_xflags & OVL_COPY_FSX_FLAGS_MASK); > > > + > > > + return ovl_real_fileattr(new, &newfa, true); > > > +} > > > + > > > static int ovl_copy_up_data(struct ovl_fs *ofs, struct path *old, > > > struct path *new, loff_t len) > > > { > > > @@ -493,20 +519,21 @@ static int ovl_link_up(struct ovl_copy_up_ctx *c) > > > static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp) > > > { > > > struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb); > > > + struct inode *inode = d_inode(c->dentry); > > > + struct path upperpath, datapath; > > > int err; > > > > > > + ovl_path_upper(c->dentry, &upperpath); > > > + if (WARN_ON(upperpath.dentry != NULL)) > > > + return -EIO; > > > + > > > + upperpath.dentry = temp; > > > + > > > /* > > > * Copy up data first and then xattrs. Writing data after > > > * xattrs will remove security.capability xattr automatically. > > > */ > > > if (S_ISREG(c->stat.mode) && !c->metacopy) { > > > - struct path upperpath, datapath; > > > - > > > - ovl_path_upper(c->dentry, &upperpath); > > > - if (WARN_ON(upperpath.dentry != NULL)) > > > - return -EIO; > > > - upperpath.dentry = temp; > > > - > > > ovl_path_lowerdata(c->dentry, &datapath); > > > err = ovl_copy_up_data(ofs, &datapath, &upperpath, > > > c->stat.size); > > > @@ -518,6 +545,14 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp) > > > if (err) > > > return err; > > > > > > + if (inode->i_flags & OVL_COPY_I_FLAGS_MASK) { > > > + /* > > > + * Copy the fileattr inode flags that are the source of already > > > + * copied i_flags (best effort). > > > + */ > > > + ovl_copy_fileattr(&c->lowerpath, &upperpath); > > > > I'm not sure this should be ignoring errors. Was this done to prevent > > regressing cases where the upper fs cannot store the flags? > > Yes. > > > Do you have a concrete example? > > Unpriv userns mount?? > Upperfs that does not support fileattr (FUSE?) Thanks, Amir.