* [PATCH] f2fs: reposition unlock_new_inode to prevent accessing invalid inode
@ 2014-08-26 10:35 Chao Yu
2014-08-28 1:47 ` Changman Lee
0 siblings, 1 reply; 4+ messages in thread
From: Chao Yu @ 2014-08-26 10:35 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel
As the race condition on the inode cache, following scenario can appear:
[Thread a] [Thread b]
->f2fs_mkdir
->f2fs_add_link
->__f2fs_add_link
->init_inode_metadata failed here
->gc_thread_func
->f2fs_gc
->do_garbage_collect
->gc_data_segment
->f2fs_iget
->iget_locked
->wait_on_inode
->unlock_new_inode
->move_data_page
->make_bad_inode
->iput
When we fail in create/symlink/mkdir/mknod/tmpfile, the new allocated inode
should be set as bad to avoid being accessed by other thread. But in above
scenario, it allows f2fs to access the invalid inode before this inode was set
as bad.
This patch fix the potential problem, and this issue was found by code review.
Signed-off-by: Chao Yu <chao2.yu@samsung.com>
---
fs/f2fs/namei.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 6b53ce9..845f1be 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -134,8 +134,8 @@ static int f2fs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
return 0;
out:
clear_nlink(inode);
- unlock_new_inode(inode);
make_bad_inode(inode);
+ unlock_new_inode(inode);
iput(inode);
alloc_nid_failed(sbi, ino);
return err;
@@ -267,8 +267,8 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
return err;
out:
clear_nlink(inode);
- unlock_new_inode(inode);
make_bad_inode(inode);
+ unlock_new_inode(inode);
iput(inode);
alloc_nid_failed(sbi, inode->i_ino);
return err;
@@ -308,8 +308,8 @@ static int f2fs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
out_fail:
clear_inode_flag(F2FS_I(inode), FI_INC_LINK);
clear_nlink(inode);
- unlock_new_inode(inode);
make_bad_inode(inode);
+ unlock_new_inode(inode);
iput(inode);
alloc_nid_failed(sbi, inode->i_ino);
return err;
@@ -354,8 +354,8 @@ static int f2fs_mknod(struct inode *dir, struct dentry *dentry,
return 0;
out:
clear_nlink(inode);
- unlock_new_inode(inode);
make_bad_inode(inode);
+ unlock_new_inode(inode);
iput(inode);
alloc_nid_failed(sbi, inode->i_ino);
return err;
@@ -688,8 +688,8 @@ release_out:
out:
f2fs_unlock_op(sbi);
clear_nlink(inode);
- unlock_new_inode(inode);
make_bad_inode(inode);
+ unlock_new_inode(inode);
iput(inode);
alloc_nid_failed(sbi, inode->i_ino);
return err;
--
2.0.0.421.g786a89d
------------------------------------------------------------------------------
Slashdot TV.
Video for Nerds. Stuff that matters.
http://tv.slashdot.org/
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] f2fs: reposition unlock_new_inode to prevent accessing invalid inode 2014-08-26 10:35 [PATCH] f2fs: reposition unlock_new_inode to prevent accessing invalid inode Chao Yu @ 2014-08-28 1:47 ` Changman Lee 2014-08-28 8:53 ` Chao Yu 0 siblings, 1 reply; 4+ messages in thread From: Changman Lee @ 2014-08-28 1:47 UTC (permalink / raw) To: Chao Yu; +Cc: Jaegeuk Kim, linux-kernel, linux-f2fs-devel Hi Chao, I agree it's correct unlock_new_inode should be located after make_bad_inode. About this scenario, I think we should check some condition if this could be occured; A inode allocated newly could be victim by gc thread. Then, f2fs_iget called by Thread A have to fail because we handled it as bad_inode in Thread B. However, f2fs_iget could still get inode. How about check it using is_bad_inode() in f2fs_iget. Thanks, On Tue, Aug 26, 2014 at 06:35:29PM +0800, Chao Yu wrote: > As the race condition on the inode cache, following scenario can appear: > [Thread a] [Thread b] > ->f2fs_mkdir > ->f2fs_add_link > ->__f2fs_add_link > ->init_inode_metadata failed here > ->gc_thread_func > ->f2fs_gc > ->do_garbage_collect > ->gc_data_segment > ->f2fs_iget > ->iget_locked > ->wait_on_inode > ->unlock_new_inode > ->move_data_page > ->make_bad_inode > ->iput > > When we fail in create/symlink/mkdir/mknod/tmpfile, the new allocated inode > should be set as bad to avoid being accessed by other thread. But in above > scenario, it allows f2fs to access the invalid inode before this inode was set > as bad. > This patch fix the potential problem, and this issue was found by code review. > > Signed-off-by: Chao Yu <chao2.yu@samsung.com> > --- > fs/f2fs/namei.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c > index 6b53ce9..845f1be 100644 > --- a/fs/f2fs/namei.c > +++ b/fs/f2fs/namei.c > @@ -134,8 +134,8 @@ static int f2fs_create(struct inode *dir, struct dentry *dentry, umode_t mode, > return 0; > out: > clear_nlink(inode); > - unlock_new_inode(inode); > make_bad_inode(inode); > + unlock_new_inode(inode); > iput(inode); > alloc_nid_failed(sbi, ino); > return err; > @@ -267,8 +267,8 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry, > return err; > out: > clear_nlink(inode); > - unlock_new_inode(inode); > make_bad_inode(inode); > + unlock_new_inode(inode); > iput(inode); > alloc_nid_failed(sbi, inode->i_ino); > return err; > @@ -308,8 +308,8 @@ static int f2fs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode) > out_fail: > clear_inode_flag(F2FS_I(inode), FI_INC_LINK); > clear_nlink(inode); > - unlock_new_inode(inode); > make_bad_inode(inode); > + unlock_new_inode(inode); > iput(inode); > alloc_nid_failed(sbi, inode->i_ino); > return err; > @@ -354,8 +354,8 @@ static int f2fs_mknod(struct inode *dir, struct dentry *dentry, > return 0; > out: > clear_nlink(inode); > - unlock_new_inode(inode); > make_bad_inode(inode); > + unlock_new_inode(inode); > iput(inode); > alloc_nid_failed(sbi, inode->i_ino); > return err; > @@ -688,8 +688,8 @@ release_out: > out: > f2fs_unlock_op(sbi); > clear_nlink(inode); > - unlock_new_inode(inode); > make_bad_inode(inode); > + unlock_new_inode(inode); > iput(inode); > alloc_nid_failed(sbi, inode->i_ino); > return err; > -- > 2.0.0.421.g786a89d > > > > ------------------------------------------------------------------------------ > Slashdot TV. > Video for Nerds. Stuff that matters. > http://tv.slashdot.org/ > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/ ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] f2fs: reposition unlock_new_inode to prevent accessing invalid inode 2014-08-28 1:47 ` Changman Lee @ 2014-08-28 8:53 ` Chao Yu 2014-08-28 10:13 ` Changman Lee 0 siblings, 1 reply; 4+ messages in thread From: Chao Yu @ 2014-08-28 8:53 UTC (permalink / raw) To: 'Changman Lee' Cc: 'Jaegeuk Kim', linux-kernel, linux-f2fs-devel Hi Changman, > -----Original Message----- > From: Changman Lee [mailto:cm224.lee@samsung.com] > Sent: Thursday, August 28, 2014 9:48 AM > To: Chao Yu > Cc: Jaegeuk Kim; linux-kernel@vger.kernel.org; linux-f2fs-devel@lists.sourceforge.net > Subject: Re: [f2fs-dev] [PATCH] f2fs: reposition unlock_new_inode to prevent accessing invalid > inode > > Hi Chao, > > I agree it's correct unlock_new_inode should be located after make_bad_inode. > > About this scenario, > I think we should check some condition if this could be occured; I think this condition is the almost impossible but which can happen theoretically. > A inode allocated newly could be victim by gc thread. > Then, f2fs_iget called by Thread A have to fail because we handled it as > bad_inode in Thread B. However, f2fs_iget could still get inode. > How about check it using is_bad_inode() in f2fs_iget. Yes, agreed. How about return -EIO when this inode we iget_locked is bad? Thanks, Yu > > Thanks, > > On Tue, Aug 26, 2014 at 06:35:29PM +0800, Chao Yu wrote: > > As the race condition on the inode cache, following scenario can appear: > > [Thread a] [Thread b] > > ->f2fs_mkdir > > ->f2fs_add_link > > ->__f2fs_add_link > > ->init_inode_metadata failed here > > ->gc_thread_func > > ->f2fs_gc > > ->do_garbage_collect > > ->gc_data_segment > > ->f2fs_iget > > ->iget_locked > > ->wait_on_inode > > ->unlock_new_inode > > ->move_data_page > > ->make_bad_inode > > ->iput > > > > When we fail in create/symlink/mkdir/mknod/tmpfile, the new allocated inode > > should be set as bad to avoid being accessed by other thread. But in above > > scenario, it allows f2fs to access the invalid inode before this inode was set > > as bad. > > This patch fix the potential problem, and this issue was found by code review. > > > > Signed-off-by: Chao Yu <chao2.yu@samsung.com> > > --- > > fs/f2fs/namei.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c > > index 6b53ce9..845f1be 100644 > > --- a/fs/f2fs/namei.c > > +++ b/fs/f2fs/namei.c > > @@ -134,8 +134,8 @@ static int f2fs_create(struct inode *dir, struct dentry *dentry, umode_t > mode, > > return 0; > > out: > > clear_nlink(inode); > > - unlock_new_inode(inode); > > make_bad_inode(inode); > > + unlock_new_inode(inode); > > iput(inode); > > alloc_nid_failed(sbi, ino); > > return err; > > @@ -267,8 +267,8 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry, > > return err; > > out: > > clear_nlink(inode); > > - unlock_new_inode(inode); > > make_bad_inode(inode); > > + unlock_new_inode(inode); > > iput(inode); > > alloc_nid_failed(sbi, inode->i_ino); > > return err; > > @@ -308,8 +308,8 @@ static int f2fs_mkdir(struct inode *dir, struct dentry *dentry, umode_t > mode) > > out_fail: > > clear_inode_flag(F2FS_I(inode), FI_INC_LINK); > > clear_nlink(inode); > > - unlock_new_inode(inode); > > make_bad_inode(inode); > > + unlock_new_inode(inode); > > iput(inode); > > alloc_nid_failed(sbi, inode->i_ino); > > return err; > > @@ -354,8 +354,8 @@ static int f2fs_mknod(struct inode *dir, struct dentry *dentry, > > return 0; > > out: > > clear_nlink(inode); > > - unlock_new_inode(inode); > > make_bad_inode(inode); > > + unlock_new_inode(inode); > > iput(inode); > > alloc_nid_failed(sbi, inode->i_ino); > > return err; > > @@ -688,8 +688,8 @@ release_out: > > out: > > f2fs_unlock_op(sbi); > > clear_nlink(inode); > > - unlock_new_inode(inode); > > make_bad_inode(inode); > > + unlock_new_inode(inode); > > iput(inode); > > alloc_nid_failed(sbi, inode->i_ino); > > return err; > > -- > > 2.0.0.421.g786a89d > > > > > > > > ------------------------------------------------------------------------------ > > Slashdot TV. > > Video for Nerds. Stuff that matters. > > http://tv.slashdot.org/ > > _______________________________________________ > > Linux-f2fs-devel mailing list > > Linux-f2fs-devel@lists.sourceforge.net > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/ ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] f2fs: reposition unlock_new_inode to prevent accessing invalid inode 2014-08-28 8:53 ` Chao Yu @ 2014-08-28 10:13 ` Changman Lee 0 siblings, 0 replies; 4+ messages in thread From: Changman Lee @ 2014-08-28 10:13 UTC (permalink / raw) To: Chao Yu; +Cc: 'Jaegeuk Kim', linux-kernel, linux-f2fs-devel Hi, On Thu, Aug 28, 2014 at 04:53:01PM +0800, Chao Yu wrote: > Hi Changman, > > > -----Original Message----- > > From: Changman Lee [mailto:cm224.lee@samsung.com] > > Sent: Thursday, August 28, 2014 9:48 AM > > To: Chao Yu > > Cc: Jaegeuk Kim; linux-kernel@vger.kernel.org; linux-f2fs-devel@lists.sourceforge.net > > Subject: Re: [f2fs-dev] [PATCH] f2fs: reposition unlock_new_inode to prevent accessing invalid > > inode > > > > Hi Chao, > > > > I agree it's correct unlock_new_inode should be located after make_bad_inode. > > > > About this scenario, > > I think we should check some condition if this could be occured; > > I think this condition is the almost impossible but which can happen theoretically. > > > A inode allocated newly could be victim by gc thread. > > Then, f2fs_iget called by Thread A have to fail because we handled it as > > bad_inode in Thread B. However, f2fs_iget could still get inode. > > How about check it using is_bad_inode() in f2fs_iget. > > Yes, agreed. How about return -EIO when this inode we iget_locked is bad? Hmm.. It might be better to check return value of f2fs_iget like other f/s. - a/fs/f2fs/gc.c +++ b/fs/f2fs/gc.c @@ -595,6 +595,8 @@ next_step: inode = f2fs_iget(sb, dni.ino); if (IS_ERR(inode)) continue; + else if (is_bad_inode(inode)) + continue; Thanks, Changman > > Thanks, > Yu > > > > > Thanks, > > > > On Tue, Aug 26, 2014 at 06:35:29PM +0800, Chao Yu wrote: > > > As the race condition on the inode cache, following scenario can appear: > > > [Thread a] [Thread b] > > > ->f2fs_mkdir > > > ->f2fs_add_link > > > ->__f2fs_add_link > > > ->init_inode_metadata failed here > > > ->gc_thread_func > > > ->f2fs_gc > > > ->do_garbage_collect > > > ->gc_data_segment > > > ->f2fs_iget > > > ->iget_locked > > > ->wait_on_inode > > > ->unlock_new_inode > > > ->move_data_page > > > ->make_bad_inode > > > ->iput > > > > > > When we fail in create/symlink/mkdir/mknod/tmpfile, the new allocated inode > > > should be set as bad to avoid being accessed by other thread. But in above > > > scenario, it allows f2fs to access the invalid inode before this inode was set > > > as bad. > > > This patch fix the potential problem, and this issue was found by code review. > > > > > > Signed-off-by: Chao Yu <chao2.yu@samsung.com> > > > --- > > > fs/f2fs/namei.c | 10 +++++----- > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c > > > index 6b53ce9..845f1be 100644 > > > --- a/fs/f2fs/namei.c > > > +++ b/fs/f2fs/namei.c > > > @@ -134,8 +134,8 @@ static int f2fs_create(struct inode *dir, struct dentry *dentry, umode_t > > mode, > > > return 0; > > > out: > > > clear_nlink(inode); > > > - unlock_new_inode(inode); > > > make_bad_inode(inode); > > > + unlock_new_inode(inode); > > > iput(inode); > > > alloc_nid_failed(sbi, ino); > > > return err; > > > @@ -267,8 +267,8 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry, > > > return err; > > > out: > > > clear_nlink(inode); > > > - unlock_new_inode(inode); > > > make_bad_inode(inode); > > > + unlock_new_inode(inode); > > > iput(inode); > > > alloc_nid_failed(sbi, inode->i_ino); > > > return err; > > > @@ -308,8 +308,8 @@ static int f2fs_mkdir(struct inode *dir, struct dentry *dentry, umode_t > > mode) > > > out_fail: > > > clear_inode_flag(F2FS_I(inode), FI_INC_LINK); > > > clear_nlink(inode); > > > - unlock_new_inode(inode); > > > make_bad_inode(inode); > > > + unlock_new_inode(inode); > > > iput(inode); > > > alloc_nid_failed(sbi, inode->i_ino); > > > return err; > > > @@ -354,8 +354,8 @@ static int f2fs_mknod(struct inode *dir, struct dentry *dentry, > > > return 0; > > > out: > > > clear_nlink(inode); > > > - unlock_new_inode(inode); > > > make_bad_inode(inode); > > > + unlock_new_inode(inode); > > > iput(inode); > > > alloc_nid_failed(sbi, inode->i_ino); > > > return err; > > > @@ -688,8 +688,8 @@ release_out: > > > out: > > > f2fs_unlock_op(sbi); > > > clear_nlink(inode); > > > - unlock_new_inode(inode); > > > make_bad_inode(inode); > > > + unlock_new_inode(inode); > > > iput(inode); > > > alloc_nid_failed(sbi, inode->i_ino); > > > return err; > > > -- > > > 2.0.0.421.g786a89d > > > > > > > > > > > > ------------------------------------------------------------------------------ > > > Slashdot TV. > > > Video for Nerds. Stuff that matters. > > > http://tv.slashdot.org/ > > > _______________________________________________ > > > Linux-f2fs-devel mailing list > > > Linux-f2fs-devel@lists.sourceforge.net > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/ ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-08-28 10:15 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-08-26 10:35 [PATCH] f2fs: reposition unlock_new_inode to prevent accessing invalid inode Chao Yu 2014-08-28 1:47 ` Changman Lee 2014-08-28 8:53 ` Chao Yu 2014-08-28 10:13 ` Changman Lee
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).