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

[Long Li <leo.lilong@huawei.com>]

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