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

[Chao Yu]

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@xxxxxxxxxx>
> ---
>   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);