On Sat, May 12, 2018 at 12:17 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote: > Currently, there is a small window where ovl_obtain_alias() can > race with ovl_instantiate() and create two aliases for an overlay > directory inode, see Al's explanation in this post: > https://marc.info/?l=linux-fsdevel&m=152599914515224&w=2 > > This patch fixes the race, by using the d_instantiate_new() helper. > > Another logic change by this patch is that if there is an > inconcsistency and a new created upper inode apears to already > exist in icache (hashed by the same real upper inode), we will > export this error to user instead of silently not hashing the new > inode. > > Backporting only makes sense for v4.16 where NFS export was introduced. > > Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx> > Cc: <stable@xxxxxxxxxxxxxxx> #v4.16 > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx> > --- > fs/overlayfs/dir.c | 27 +++++++++++++++++---------- > fs/overlayfs/inode.c | 6 ++++++ > fs/overlayfs/overlayfs.h | 1 + > 3 files changed, 24 insertions(+), 10 deletions(-) > > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c > index 47dc980e8b33..c41825a5ed5f 100644 > --- a/fs/overlayfs/dir.c > +++ b/fs/overlayfs/dir.c > @@ -183,23 +183,30 @@ static int ovl_set_opaque(struct dentry *dentry, struct dentry *upperdentry) > } > > /* Common operations required to be done after creation of file on upper */ > -static void ovl_instantiate(struct dentry *dentry, struct inode *inode, > - struct dentry *newdentry, bool hardlink) > +static int ovl_instantiate(struct dentry *dentry, struct inode *inode, > + struct dentry *newdentry, bool hardlink) > { > ovl_dir_modified(dentry->d_parent, false); > - ovl_copyattr(d_inode(newdentry), inode); Miklos, Note that removal of ovl_copyattr() here (for hardlink case) causes xfstest overlay/019 to hit WARN_ON in ovl_create_or_link() about 2 minutes into the test and causes the test to fail. WARN_ON(!uid_eq(inode->i_uid, realinode->i_uid)); WARN_ON(!gid_eq(inode->i_gid, realinode->i_gid)); This is a stress tests that makes modification to underlying layer while overlay is mounted, so it is not surprising. I think the correct solution is to turn those WARN_ON to pr_warn_ratelimited(). There are many of those fired by the stress test. Do you agree this is the correct fix? Thanks, Amir. > ovl_dentry_set_upper_alias(dentry); > if (!hardlink) { > - ovl_inode_update(inode, newdentry); > + int err; > + > + ovl_inode_init(inode, newdentry, NULL); > + err = ovl_insert_inode_locked(inode, d_inode(newdentry)); > + if (err) > + return err; > + > + d_instantiate_new(dentry, inode); > } else { > WARN_ON(ovl_inode_real(inode) != d_inode(newdentry)); > dput(newdentry); > inc_nlink(inode); > - } > - d_instantiate(dentry, inode); > - /* Force lookup of new upper hardlink to find its lower */ > - if (hardlink) > + d_instantiate(dentry, inode); > + /* Force lookup of new upper hardlink to find its lower */ > d_drop(dentry); > + } > + > + return 0; > } > > static bool ovl_type_merge(struct dentry *dentry) > @@ -238,7 +245,7 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode, > ovl_set_opaque(dentry, newdentry); > } > > - ovl_instantiate(dentry, inode, newdentry, !!hardlink); > + err = ovl_instantiate(dentry, inode, newdentry, !!hardlink); > newdentry = NULL; > out_dput: > dput(newdentry); > @@ -439,7 +446,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode, > if (err) > goto out_cleanup; > } > - ovl_instantiate(dentry, inode, newdentry, !!hardlink); > + err = ovl_instantiate(dentry, inode, newdentry, !!hardlink); > newdentry = NULL; > out_dput2: > dput(upper); > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c > index 7abcf96e94fc..70c966b1bb5a 100644 > --- a/fs/overlayfs/inode.c > +++ b/fs/overlayfs/inode.c > @@ -741,6 +741,12 @@ static bool ovl_verify_inode(struct inode *inode, struct dentry *lowerdentry, > return true; > } > > +int ovl_insert_inode_locked(struct inode *inode, struct inode *realinode) > +{ > + return insert_inode_locked4(inode, (unsigned long) realinode, > + ovl_inode_test, realinode); > +} > + > struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real, > bool is_upper) > { > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h > index caaa47cea2aa..dd320be86600 100644 > --- a/fs/overlayfs/overlayfs.h > +++ b/fs/overlayfs/overlayfs.h > @@ -343,6 +343,7 @@ int ovl_update_time(struct inode *inode, struct timespec *ts, int flags); > bool ovl_is_private_xattr(const char *name); > > struct inode *ovl_new_inode(struct super_block *sb, umode_t mode, dev_t rdev); > +int ovl_insert_inode_locked(struct inode *inode, struct inode *realinode); > struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real, > bool is_upper); > struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry, > -- > 2.7.4 >
