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.lil...@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
