[f2fs-dev] [PATCH 1/2] f2fs: fix race in concurrent f2fs_stop_gc_thread

[f2fs-dev] [PATCH 1/2] f2fs: fix race in concurrent f2fs_stop_gc_thread

[Long Li via Linux-f2fs-devel]

In my test case, concurrent calls to f2fs shutdown report the following
stack trace:

 Oops: general protection fault, probably for non-canonical address 0xc6cfff63bb5513fc: 0000 [#1] PREEMPT SMP PTI
 CPU: 0 UID: 0 PID: 678 Comm: f2fs_rep_shutdo Not tainted 6.12.0-rc5-next-20241029-g6fb2fa9805c5-dirty #85
 Call Trace:
  <TASK>
  ? show_regs+0x8b/0xa0
  ? __die_body+0x26/0xa0
  ? die_addr+0x54/0x90
  ? exc_general_protection+0x24b/0x5c0
  ? asm_exc_general_protection+0x26/0x30
  ? kthread_stop+0x46/0x390
  f2fs_stop_gc_thread+0x6c/0x110
  f2fs_do_shutdown+0x309/0x3a0
  f2fs_ioc_shutdown+0x150/0x1c0
  __f2fs_ioctl+0xffd/0x2ac0
  f2fs_ioctl+0x76/0xe0
  vfs_ioctl+0x23/0x60
  __x64_sys_ioctl+0xce/0xf0
  x64_sys_call+0x2b1b/0x4540
  do_syscall_64+0xa7/0x240
  entry_SYSCALL_64_after_hwframe+0x76/0x7e

The root cause is a race condition in f2fs_stop_gc_thread() called from
different f2fs shutdown paths:

  [CPU0]                       [CPU1]
  ----------------------       -----------------------
  f2fs_stop_gc_thread          f2fs_stop_gc_thread
                                 gc_th = sbi->gc_thread
    gc_th = sbi->gc_thread
    kfree(gc_th)
    sbi->gc_thread = NULL
                                 < gc_th != NULL >
                                 kthread_stop(gc_th->f2fs_gc_task) //UAF

The commit c7f114d864ac ("f2fs: fix to avoid use-after-free in
f2fs_stop_gc_thread()") attempted to fix this issue by using a read
semaphore to prevent races between shutdown and remount threads, but
itfails to prevent all race conditions.

While upgrading to s_umount write lock in f2fs_do_shutdown() would fix
the current issue, however, using s_umount lock requires extreme caution
to avoid lock recursion. A better solution is to introduce a semaphore
to prevent races between concurrent f2fs_stop_gc_thread calls.

Fixes: 7950e9ac638e ("f2fs: stop gc/discard thread after fs shutdown")
Signed-off-by: Long Li <leo.lilong@huawei.com>
---
 fs/f2fs/f2fs.h  | 1 +
 fs/f2fs/gc.c    | 9 +++++++--
 fs/f2fs/super.c | 1 +
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 3c6f3cce5779..7ae1e2a4789f 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1679,6 +1679,7 @@ struct f2fs_sb_info {
 						 * race between GC and GC or CP
 						 */
 	struct f2fs_gc_kthread	*gc_thread;	/* GC thread */
+	struct semaphore gc_clean_lock;		/* semaphore for clean GC thread */
 	struct atgc_management am;		/* atgc management */
 	unsigned int cur_victim_sec;		/* current victim section num */
 	unsigned int gc_mode;			/* current GC state */
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index e40bdd12e36d..e1b8bf98b5fa 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -232,14 +232,19 @@ int f2fs_start_gc_thread(struct f2fs_sb_info *sbi)
 
 void f2fs_stop_gc_thread(struct f2fs_sb_info *sbi)
 {
-	struct f2fs_gc_kthread *gc_th = sbi->gc_thread;
+	struct f2fs_gc_kthread *gc_th;
 
-	if (!gc_th)
+	down(&sbi->gc_clean_lock);
+	gc_th = sbi->gc_thread;
+	if (!gc_th) {
+		up(&sbi->gc_clean_lock);
 		return;
+	}
 	kthread_stop(gc_th->f2fs_gc_task);
 	wake_up_all(&gc_th->fggc_wq);
 	kfree(gc_th);
 	sbi->gc_thread = NULL;
+	up(&sbi->gc_clean_lock);
 }
 
 static int select_gc_type(struct f2fs_sb_info *sbi, int gc_type)
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 80a53dbf1c38..47a15050ea9c 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -4419,6 +4419,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 
 	/* initialize locks within allocated memory */
 	init_f2fs_rwsem(&sbi->gc_lock);
+	sema_init(&sbi->gc_clean_lock, 1);
 	mutex_init(&sbi->writepages);
 	init_f2fs_rwsem(&sbi->cp_global_sem);
 	init_f2fs_rwsem(&sbi->node_write);
-- 
2.39.2



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH 2/2] Revert "f2fs: fix to avoid use-after-free in f2fs_stop_gc_thread()"

[Long Li via Linux-f2fs-devel]

This reverts commit c7f114d864ac91515bb07ac271e9824a20f5ed95.

The race conditions between concurrent f2fs_stop_gc_thread() calls
are now protected by a dedicated lock, making the additional s_umount
lock protection unnecessary. Therefore, revert this patch.

Signed-off-by: Long Li <leo.lilong@huawei.com>
---
 fs/f2fs/f2fs.h  |  2 +-
 fs/f2fs/file.c  | 11 ++---------
 fs/f2fs/super.c |  2 +-
 3 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 7ae1e2a4789f..2143604ce416 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3522,7 +3522,7 @@ int f2fs_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
 int f2fs_truncate_hole(struct inode *inode, pgoff_t pg_start, pgoff_t pg_end);
 void f2fs_truncate_data_blocks_range(struct dnode_of_data *dn, int count);
 int f2fs_do_shutdown(struct f2fs_sb_info *sbi, unsigned int flag,
-						bool readonly, bool need_lock);
+							bool readonly);
 int f2fs_precache_extents(struct inode *inode);
 int f2fs_fileattr_get(struct dentry *dentry, struct fileattr *fa);
 int f2fs_fileattr_set(struct mnt_idmap *idmap,
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 75a8b22da664..5d7b4fdae9c4 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2318,7 +2318,7 @@ static int f2fs_ioc_abort_atomic_write(struct file *filp)
 }
 
 int f2fs_do_shutdown(struct f2fs_sb_info *sbi, unsigned int flag,
-						bool readonly, bool need_lock)
+							bool readonly)
 {
 	struct super_block *sb = sbi->sb;
 	int ret = 0;
@@ -2365,19 +2365,12 @@ int f2fs_do_shutdown(struct f2fs_sb_info *sbi, unsigned int flag,
 	if (readonly)
 		goto out;
 
-	/* grab sb->s_umount to avoid racing w/ remount() */
-	if (need_lock)
-		down_read(&sbi->sb->s_umount);
-
 	f2fs_stop_gc_thread(sbi);
 	f2fs_stop_discard_thread(sbi);
 
 	f2fs_drop_discard_cmd(sbi);
 	clear_opt(sbi, DISCARD);
 
-	if (need_lock)
-		up_read(&sbi->sb->s_umount);
-
 	f2fs_update_time(sbi, REQ_TIME);
 out:
 
@@ -2414,7 +2407,7 @@ static int f2fs_ioc_shutdown(struct file *filp, unsigned long arg)
 		}
 	}
 
-	ret = f2fs_do_shutdown(sbi, in, readonly, true);
+	ret = f2fs_do_shutdown(sbi, in, readonly);
 
 	if (need_drop)
 		mnt_drop_write_file(filp);
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 47a15050ea9c..a720fb9ef196 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -2569,7 +2569,7 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data)
 
 static void f2fs_shutdown(struct super_block *sb)
 {
-	f2fs_do_shutdown(F2FS_SB(sb), F2FS_GOING_DOWN_NOSYNC, false, false);
+	f2fs_do_shutdown(F2FS_SB(sb), F2FS_GOING_DOWN_NOSYNC, false);
 }
 
 #ifdef CONFIG_QUOTA
-- 
2.39.2



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 1/2] f2fs: fix race in concurrent f2fs_stop_gc_thread

[Chao Yu via Linux-f2fs-devel]

On 2024/10/31 14:45, Long Li wrote:
> In my test case, concurrent calls to f2fs shutdown report the following
> stack trace:
> 
>   Oops: general protection fault, probably for non-canonical address 0xc6cfff63bb5513fc: 0000 [#1] PREEMPT SMP PTI
>   CPU: 0 UID: 0 PID: 678 Comm: f2fs_rep_shutdo Not tainted 6.12.0-rc5-next-20241029-g6fb2fa9805c5-dirty #85
>   Call Trace:
>    <TASK>
>    ? show_regs+0x8b/0xa0
>    ? __die_body+0x26/0xa0
>    ? die_addr+0x54/0x90
>    ? exc_general_protection+0x24b/0x5c0
>    ? asm_exc_general_protection+0x26/0x30
>    ? kthread_stop+0x46/0x390
>    f2fs_stop_gc_thread+0x6c/0x110
>    f2fs_do_shutdown+0x309/0x3a0
>    f2fs_ioc_shutdown+0x150/0x1c0
>    __f2fs_ioctl+0xffd/0x2ac0
>    f2fs_ioctl+0x76/0xe0
>    vfs_ioctl+0x23/0x60
>    __x64_sys_ioctl+0xce/0xf0
>    x64_sys_call+0x2b1b/0x4540
>    do_syscall_64+0xa7/0x240
>    entry_SYSCALL_64_after_hwframe+0x76/0x7e
> 
> The root cause is a race condition in f2fs_stop_gc_thread() called from
> different f2fs shutdown paths:
> 
>    [CPU0]                       [CPU1]
>    ----------------------       -----------------------
>    f2fs_stop_gc_thread          f2fs_stop_gc_thread
>                                   gc_th = sbi->gc_thread
>      gc_th = sbi->gc_thread
>      kfree(gc_th)
>      sbi->gc_thread = NULL
>                                   < gc_th != NULL >
>                                   kthread_stop(gc_th->f2fs_gc_task) //UAF

Hi Long,

Thanks for catching this.

Can we cover f2fs_stop_gc_thread() w/ write lock of s_umount in
f2fs_do_shutdown()?

Thanks,

> 
> The commit c7f114d864ac ("f2fs: fix to avoid use-after-free in
> f2fs_stop_gc_thread()") attempted to fix this issue by using a read
> semaphore to prevent races between shutdown and remount threads, but
> itfails to prevent all race conditions.
> 
> While upgrading to s_umount write lock in f2fs_do_shutdown() would fix
> the current issue, however, using s_umount lock requires extreme caution
> to avoid lock recursion. A better solution is to introduce a semaphore
> to prevent races between concurrent f2fs_stop_gc_thread calls.
> 
> Fixes: 7950e9ac638e ("f2fs: stop gc/discard thread after fs shutdown")
> Signed-off-by: Long Li <leo.lilong@huawei.com>
> ---
>   fs/f2fs/f2fs.h  | 1 +
>   fs/f2fs/gc.c    | 9 +++++++--
>   fs/f2fs/super.c | 1 +
>   3 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 3c6f3cce5779..7ae1e2a4789f 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1679,6 +1679,7 @@ struct f2fs_sb_info {
>   						 * race between GC and GC or CP
>   						 */
>   	struct f2fs_gc_kthread	*gc_thread;	/* GC thread */
> +	struct semaphore gc_clean_lock;		/* semaphore for clean GC thread */
>   	struct atgc_management am;		/* atgc management */
>   	unsigned int cur_victim_sec;		/* current victim section num */
>   	unsigned int gc_mode;			/* current GC state */
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index e40bdd12e36d..e1b8bf98b5fa 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -232,14 +232,19 @@ int f2fs_start_gc_thread(struct f2fs_sb_info *sbi)
>   
>   void f2fs_stop_gc_thread(struct f2fs_sb_info *sbi)
>   {
> -	struct f2fs_gc_kthread *gc_th = sbi->gc_thread;
> +	struct f2fs_gc_kthread *gc_th;
>   
> -	if (!gc_th)
> +	down(&sbi->gc_clean_lock);
> +	gc_th = sbi->gc_thread;
> +	if (!gc_th) {
> +		up(&sbi->gc_clean_lock);
>   		return;
> +	}
>   	kthread_stop(gc_th->f2fs_gc_task);
>   	wake_up_all(&gc_th->fggc_wq);
>   	kfree(gc_th);
>   	sbi->gc_thread = NULL;
> +	up(&sbi->gc_clean_lock);
>   }
>   
>   static int select_gc_type(struct f2fs_sb_info *sbi, int gc_type)
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 80a53dbf1c38..47a15050ea9c 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -4419,6 +4419,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>   
>   	/* initialize locks within allocated memory */
>   	init_f2fs_rwsem(&sbi->gc_lock);
> +	sema_init(&sbi->gc_clean_lock, 1);
>   	mutex_init(&sbi->writepages);
>   	init_f2fs_rwsem(&sbi->cp_global_sem);
>   	init_f2fs_rwsem(&sbi->node_write);



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 1/2] f2fs: fix race in concurrent f2fs_stop_gc_thread

[Long Li via Linux-f2fs-devel]

On Fri, Nov 01, 2024 at 10:56:59AM +0800, Chao Yu wrote:
> On 2024/10/31 14:45, Long Li wrote:
> > In my test case, concurrent calls to f2fs shutdown report the following
> > stack trace:
> > 
> >   Oops: general protection fault, probably for non-canonical address 0xc6cfff63bb5513fc: 0000 [#1] PREEMPT SMP PTI
> >   CPU: 0 UID: 0 PID: 678 Comm: f2fs_rep_shutdo Not tainted 6.12.0-rc5-next-20241029-g6fb2fa9805c5-dirty #85
> >   Call Trace:
> >    <TASK>
> >    ? show_regs+0x8b/0xa0
> >    ? __die_body+0x26/0xa0
> >    ? die_addr+0x54/0x90
> >    ? exc_general_protection+0x24b/0x5c0
> >    ? asm_exc_general_protection+0x26/0x30
> >    ? kthread_stop+0x46/0x390
> >    f2fs_stop_gc_thread+0x6c/0x110
> >    f2fs_do_shutdown+0x309/0x3a0
> >    f2fs_ioc_shutdown+0x150/0x1c0
> >    __f2fs_ioctl+0xffd/0x2ac0
> >    f2fs_ioctl+0x76/0xe0
> >    vfs_ioctl+0x23/0x60
> >    __x64_sys_ioctl+0xce/0xf0
> >    x64_sys_call+0x2b1b/0x4540
> >    do_syscall_64+0xa7/0x240
> >    entry_SYSCALL_64_after_hwframe+0x76/0x7e
> > 
> > The root cause is a race condition in f2fs_stop_gc_thread() called from
> > different f2fs shutdown paths:
> > 
> >    [CPU0]                       [CPU1]
> >    ----------------------       -----------------------
> >    f2fs_stop_gc_thread          f2fs_stop_gc_thread
> >                                   gc_th = sbi->gc_thread
> >      gc_th = sbi->gc_thread
> >      kfree(gc_th)
> >      sbi->gc_thread = NULL
> >                                   < gc_th != NULL >
> >                                   kthread_stop(gc_th->f2fs_gc_task) //UAF
> 
> Hi Long,
> 
> Thanks for catching this.
> 
> Can we cover f2fs_stop_gc_thread() w/ write lock of s_umount in
> f2fs_do_shutdown()?
> 
> Thanks,
> 

Hi Chao,

If you think holding write lock inf2fs_do_shutdown() is better, I'll
send v2 with this change.

Thanks,
Long Li


> > 
> > The commit c7f114d864ac ("f2fs: fix to avoid use-after-free in
> > f2fs_stop_gc_thread()") attempted to fix this issue by using a read
> > semaphore to prevent races between shutdown and remount threads, but
> > itfails to prevent all race conditions.
> > 
> > While upgrading to s_umount write lock in f2fs_do_shutdown() would fix
> > the current issue, however, using s_umount lock requires extreme caution
> > to avoid lock recursion. A better solution is to introduce a semaphore
> > to prevent races between concurrent f2fs_stop_gc_thread calls.
> > 
> > Fixes: 7950e9ac638e ("f2fs: stop gc/discard thread after fs shutdown")
> > Signed-off-by: Long Li <leo.lilong@huawei.com>
> > ---
> >   fs/f2fs/f2fs.h  | 1 +
> >   fs/f2fs/gc.c    | 9 +++++++--
> >   fs/f2fs/super.c | 1 +
> >   3 files changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index 3c6f3cce5779..7ae1e2a4789f 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -1679,6 +1679,7 @@ struct f2fs_sb_info {
> >   						 * race between GC and GC or CP
> >   						 */
> >   	struct f2fs_gc_kthread	*gc_thread;	/* GC thread */
> > +	struct semaphore gc_clean_lock;		/* semaphore for clean GC thread */
> >   	struct atgc_management am;		/* atgc management */
> >   	unsigned int cur_victim_sec;		/* current victim section num */
> >   	unsigned int gc_mode;			/* current GC state */
> > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > index e40bdd12e36d..e1b8bf98b5fa 100644
> > --- a/fs/f2fs/gc.c
> > +++ b/fs/f2fs/gc.c
> > @@ -232,14 +232,19 @@ int f2fs_start_gc_thread(struct f2fs_sb_info *sbi)
> >   void f2fs_stop_gc_thread(struct f2fs_sb_info *sbi)
> >   {
> > -	struct f2fs_gc_kthread *gc_th = sbi->gc_thread;
> > +	struct f2fs_gc_kthread *gc_th;
> > -	if (!gc_th)
> > +	down(&sbi->gc_clean_lock);
> > +	gc_th = sbi->gc_thread;
> > +	if (!gc_th) {
> > +		up(&sbi->gc_clean_lock);
> >   		return;
> > +	}
> >   	kthread_stop(gc_th->f2fs_gc_task);
> >   	wake_up_all(&gc_th->fggc_wq);
> >   	kfree(gc_th);
> >   	sbi->gc_thread = NULL;
> > +	up(&sbi->gc_clean_lock);
> >   }
> >   static int select_gc_type(struct f2fs_sb_info *sbi, int gc_type)
> > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > index 80a53dbf1c38..47a15050ea9c 100644
> > --- a/fs/f2fs/super.c
> > +++ b/fs/f2fs/super.c
> > @@ -4419,6 +4419,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> >   	/* initialize locks within allocated memory */
> >   	init_f2fs_rwsem(&sbi->gc_lock);
> > +	sema_init(&sbi->gc_clean_lock, 1);
> >   	mutex_init(&sbi->writepages);
> >   	init_f2fs_rwsem(&sbi->cp_global_sem);
> >   	init_f2fs_rwsem(&sbi->node_write);
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel