| Message ID | 1526116631-24186-2-git-send-email-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, May 12, 2018 at 12:17 PM, Amir Goldstein <amir73il@gmail.com> 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@zeniv.linux.org.uk> > Cc: <stable@vger.kernel.org> #v4.16 > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > 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 >
On Sat, May 12, 2018 at 12:17:09PM +0300, Amir Goldstein 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. Hi Amir, So for ovlerlay, this race does not exist for directories as you fall back to lookup path and don't call ovl_obtain_alias(). Does that mean it exists for non-dir only and that's what you are trying to fix. I would be nice to add few more lines describing exact race. It makes it easier and one does not have to find another email thread and dig out a specific email to figure out what was the race exactly and how does apply to overlayfs. Thanks Vivek > > Cc: Al Viro <viro@zeniv.linux.org.uk> > Cc: <stable@vger.kernel.org> #v4.16 > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > 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); > 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 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 14, 2018 at 11:30 PM, Vivek Goyal <vgoyal@redhat.com> wrote: > On Sat, May 12, 2018 at 12:17:09PM +0300, Amir Goldstein 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. > > Hi Amir, > > So for ovlerlay, this race does not exist for directories as you fall > back to lookup path and don't call ovl_obtain_alias(). Does that mean it > exists for non-dir only and that's what you are trying to fix. Hi Vivek, Thanks for being alert. You are right about the fact that overlay doesn't call ovl_obtain_alias() for dir and no, this race is not relevant for non-dir, because non-dir may have more than one hashed alias. However, I'm afraid a race does exists with ovl_get_dentry() vs. ovl_instantiate(). The race can end up with two different inodes for the same directory. This is what ovl_insert_inode_locked() fixes. I am not sure if there is also a race with inserting dentry to cache I'll need to look closer. > > I would be nice to add few more lines describing exact race. It makes > it easier and one does not have to find another email thread and > dig out a specific email to figure out what was the race exactly and > how does apply to overlayfs. > Well, I did link to the exact post with the description, although the race Al explains about is completely different than the inode insert race, so I'll need to revise the commit message. Thanks, Amir.
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); 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,
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@zeniv.linux.org.uk> Cc: <stable@vger.kernel.org> #v4.16 Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/overlayfs/dir.c | 27 +++++++++++++++++---------- fs/overlayfs/inode.c | 6 ++++++ fs/overlayfs/overlayfs.h | 1 + 3 files changed, 24 insertions(+), 10 deletions(-)