[PATCH] f2fs: reposition unlock_new_inode to prevent accessing invalid inode

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH] f2fs: reposition unlock_new_inode to prevent accessing invalid inode
@ 2014-08-26 10:35 ` Chao Yu
  0 siblings, 0 replies; 8+ 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] 8+ messages in thread

* [f2fs-dev][PATCH] f2fs: reposition unlock_new_inode to prevent accessing invalid inode
@ 2014-08-26 10:35 ` Chao Yu
  0 siblings, 0 replies; 8+ messages in thread
From: Chao Yu @ 2014-08-26 10:35 UTC (permalink / raw)
  To: Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel

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



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] f2fs: reposition unlock_new_inode to prevent accessing invalid inode
  2014-08-26 10:35 ` [f2fs-dev][PATCH] " Chao Yu
@ 2014-08-28  1:47   ` Changman Lee
  -1 siblings, 0 replies; 8+ 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] 8+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: reposition unlock_new_inode to prevent accessing invalid inode
@ 2014-08-28  1:47   ` Changman Lee
  0 siblings, 0 replies; 8+ 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] f2fs: reposition unlock_new_inode to prevent accessing invalid inode
  2014-08-28  1:47   ` [f2fs-dev] " Changman Lee
@ 2014-08-28  8:53     ` Chao Yu
  -1 siblings, 0 replies; 8+ 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] 8+ messages in thread

* RE: [f2fs-dev] [PATCH] f2fs: reposition unlock_new_inode to prevent accessing invalid inode
@ 2014-08-28  8:53     ` Chao Yu
  0 siblings, 0 replies; 8+ 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] f2fs: reposition unlock_new_inode to prevent accessing invalid inode
  2014-08-28  8:53     ` [f2fs-dev] " Chao Yu
@ 2014-08-28 10:13       ` Changman Lee
  -1 siblings, 0 replies; 8+ 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] 8+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: reposition unlock_new_inode to prevent accessing invalid inode
@ 2014-08-28 10:13       ` Changman Lee
  0 siblings, 0 replies; 8+ 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2014-08-28 10:15 UTC | newest]

Thread overview: 8+ 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-26 10:35 ` [f2fs-dev][PATCH] " Chao Yu
2014-08-28  1:47 ` [PATCH] " Changman Lee
2014-08-28  1:47   ` [f2fs-dev] " Changman Lee
2014-08-28  8:53   ` Chao Yu
2014-08-28  8:53     ` [f2fs-dev] " Chao Yu
2014-08-28 10:13     ` Changman Lee
2014-08-28 10:13       ` [f2fs-dev] " Changman Lee

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.