On Fri, Jun 18, 2021 at 4:57 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote: > > On Thu, 17 Jun 2021 at 17:22, Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > Instead, if any of the fileattr flags of interest exist on the lower > > inode, we store them in overlay.xflags xattr on the upper inode and we > > we read the flags from xattr on lookup and on fileattr_get(). > > Calling this xflags, especially near fileattr code, makes it easy to > confuse with fsx_xflags. Can we find a more distinctive name? > Indeed. I'll change to overlay.protected as suggested in the v1 patch discussion. > > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c > > index aec353a2dc80..d66e51b9c347 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 immutable/append-only STATX flags */ > > + if (ovl_test_flag(OVL_XFLAGS, inode)) > > + ovl_fill_xflags(inode, stat, NULL); > > + > > Filesystems are doing these transformations: (already down one from > before fileattr) > > internal flags -> statx->attributes > internal flags -> inode->i_flags > internal flags <-> fa->flags or fa->fsx_xflags > > To further improve this situation the statx filling could be moved to > generic code based on i_flags. I'm not asking you to convert all > filesystems (though that would be nice), but adding the helpers and > using them here would be a good first step. > I am afraid that the only flags this would be relevant to are (a) and (i), so not sure it is worth the generic helper, but I will look into it. > > @@ -639,6 +642,174 @@ int ovl_set_impure(struct dentry *dentry, struct dentry *upperdentry) > > return err; > > } > > > > + > > +/* > > + * Overlayfs stores immutable/append-only attributes in overlay.xflags xattr. > > + * If upper inode does have those fileattr flags set (i.e. from old kernel), > > + * overlayfs does not clear them on fileattr_get(), but it will clear them on > > + * fileattr_set(). > > + */ > > +#define OVL_XFLAG(c, x) \ > > + { c, S_ ## x, FS_ ## x ## _FL, FS_XFLAG_ ## x, STATX_ATTR_ ## x } > > + > > +struct ovl_xflag { > > + char code; > > + u32 i_flag; > > + u32 fs_flag; > > + u32 fsx_flag; > > + u64 statx_attr; > > +} const ovl_xflags[] = { > > + OVL_XFLAG('a', APPEND), > > + OVL_XFLAG('i', IMMUTABLE), > > +}; > > This would be really nice for a dozen flags, but for two... > > My guess is that many lines of code could be saved by un-generalizing this. > Perhaps. I'll try. > > +/* Set inode flags and xflags xattr from fileattr */ > > +int ovl_set_xflags(struct inode *inode, struct dentry *upper, > > + struct fileattr *fa) > > +{ > > + struct ovl_fs *ofs = OVL_FS(inode->i_sb); > > + char buf[OVL_XFLAGS_NUM]; > > + int len, err = 0; > > + > > + BUILD_BUG_ON(OVL_XFLAGS_NUM >= OVL_XFLAGS_MAX); > > + len = ovl_xflags_to_buf(inode, buf, OVL_XFLAGS_NUM, fa); > > + > > + /* > > + * Do not fail when upper doesn't support xattrs, but also do not > > + * mask out the xattr xflags from real fileattr to continue > > + * supporting fileattr_set() on fs without xattr support. > > + * Remove xattr if it exist and all flags are cleared. > > + */ > > Does this matter in practice? I.e. is there any filesystem with > immutable/append attribute but not xattr that could be an upper layer? I did not find any, but did not want to take the risk, because maybe there is a fs that does not support trusted.xattr but does support fileattr - I did not check that. And the fallback seemed pretty safe to me. I am also fine with failing fileattr_set() in that case. > > If yes, then this could end up as a copy-up regression (failure to > copy up files having immutable/append). No, because ovl_copy_xflags() always masks those flags on copy up: 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); The comment above about not clearing the immutable flag is referring specifically to fileattr_set(). Thanks, Amir.