From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id C402A3806D9 for ; Tue, 14 Jul 2026 02:38:30 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783996712; cv=none; b=UgqIq+axF3Ripqmu/O4TrI4UFWz6O6EOIWqTrFOwzYaU/kgnlurAv/HPA7IKFlqfT88si2EjGiMZ2ZK4eSJpWqGmwKQycHXsBFASm3NbczCx2HUB0LACp9BREtPNM2kxPpyZTzo5vgfUNj3utzp6ZldXA6TYaTXw4fMhj7nID+Y= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783996712; c=relaxed/simple; bh=hx8H/qeAwABbEsoxlb3RrXLDnyio0J49POyy9EV9sig=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Opv1O2YZrodfkfOdSh02a6o0HwAXqvDAoWPZ2hHvWQ/UoBVa6XxuSmTzrJfRoA5xPGjFLksnx51vWerHA4EactNVCcj01EtjOL7JlqJ0cTHAu/Jgl11SUouCxqWn3XDGqEdi4OwOpMqUR3vc3mpL2hbxwop6Pwr6REd9oVuf1hk= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=Uik5BOEz; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="Uik5BOEz" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 3A2741F000E9; Tue, 14 Jul 2026 02:38:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1783996710; bh=Fw+xe3f7RClCVgszAuH9YldIuwV/gLDcxiThpIVhvBY=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=Uik5BOEzDVrmkBBI9MSzMtKsCAiklLh3MzH4A7U2PJVHHg1Rmmb8ieS4G7wC8Bxnz XyK8Ii8GQPjqGSghEFMcqNbE7mk5iu+Fwe88brq1tjNU8PGh+PcoBi1uJdWdm31F5M 08JcxMRzctSe9PZv4ueRhJMNW8s/C71t3etLR2/FwhnUlJHQVQNknfqB4rX5zau6X1 6f8ebMgCyqVbkhTQvQoUtkUf0uSVvJ1mZ5gwdJZsixgdyED34SJTWjkFQ7ZtCKGIaU qKYhfFyZszAoU+dS0n3o7HudbaQL31amyGQ7PCwmPqk6t6XLuLpKz7WIUzEYVc7/Tg nj7Wj9QnY6iOg== Date: Tue, 14 Jul 2026 02:38:28 +0000 From: Jaegeuk Kim To: Cen Zhang Cc: Chao Yu , linux-f2fs-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org Subject: Re: [PATCH v7] f2fs: protect gc task pointer during teardown Message-ID: References: <20260630142046.3145107-1-rollkingzzc@gmail.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Could you please post the original patch again? Somehow I can't find it. On 07/08, Cen Zhang wrote: > Hi all, > > This is a gentle ping on this patch. > > I would really appreciate any feedback when you have a chance. > > Best regards, > Zhang Cen > > Zhang Cen 于2026年6月30日周二 22:20写道： > > > > This patch is based on the preceding patch "f2fs: embed > > f2fs_gc_kthread in f2fs_sb_info", which gives the GC waitqueues and > > thread tunables superblock lifetime. With that container lifetime fixed, > > f2fs_gc_task remains the running-state marker and the task_struct pointer > > that needs separate lifetime protection. > > > > During shutdown, f2fs_stop_gc_thread() stops the GC task and wakes > > GC_MERGE foreground waiters while f2fs_balance_fs() can concurrently > > queue on gc_th->fggc_wq. The preceding embedded-gc_thread patch keeps > > that waitqueue storage alive for the superblock lifetime; this patch > > protects the remaining f2fs_gc_task pointer by publishing and detaching > > it under gc_task_lock, and by making sysfs users that dereference the > > task hold the same lock. > > > > The buggy scenario involves two paths, with each column showing the > > order within that path: > > > > foreground f2fs_balance_fs() caller: shutdown path: > > 1. observes no checkpoint error 1. sets CP_ERROR_FLAG > > 2. snapshots sbi->gc_thread 2. enters f2fs_stop_gc_thread() > > 3. queues on gc_th->fggc_wq 3. stops gc_th->f2fs_gc_task > > 4. wakes gc_wait_queue_head 4. wakes gc_th->fggc_wq > > 5. sleeps for foreground GC 5. frees gc_th in the old layout > > 6. finish_wait() touches fggc_wq > > > > GC_MERGE does not keep independent work_struct items that shutdown can > > cancel. Its pending foreground GC requests are waitqueue waiters. Drain > > them by withdrawing the GC task pointer, stopping the task, waking > > gc_th->fggc_wq, and leaving each waiter to remove its own wait entry > > with finish_wait(). > > > > Add gc_task_lock to struct f2fs_gc_kthread and use f2fs_update_gc_task() > > to publish the GC task only after the new kthread has been created and > > its nice value has been set. The start path uses kthread_create() so the > > task is not woken until after f2fs_gc_task is visible to waiters. The > > stop path updates f2fs_gc_task to NULL under the same lock before > > kthread_stop(), so later readers see that no new foreground GC work > > should be handed to the background thread. > > > > f2fs_balance_fs() also rechecks f2fs_gc_task after prepare_to_wait(). If > > the GC task has already been withdrawn, the caller removes its wait entry > > without waking the GC thread or sleeping for new foreground GC work. Thus > > shutdown drains already queued waiters and stops accepting new foreground > > GC work once the task pointer is withdrawn. > > > > Task pointer users are protected separately from the embedded container > > lifetime. A sysfs critical_task_priority store now checks f2fs_gc_task > > and calls set_user_nice() under gc_task_lock. Boolean running-state > > checks that do not dereference the task_struct use READ_ONCE(). > > > > One observed report was: > > > > BUG: KASAN: slab-use-after-free in finish_wait+0x276/0x290 > > Write of size 8 at addr ffff8881150819b8 by task dd/802 > > The buggy address belongs to the object at ffff888115081900 which > > belongs to the cache kmalloc-256 of size 256 > > The buggy address is located 184 bytes inside of freed 256-byte region > > Call trace: > > finish_wait() > > f2fs_balance_fs() > > f2fs_write_single_data_page() > > f2fs_write_cache_pages() > > __f2fs_write_data_pages() > > do_writepages() > > filemap_fdatawrite_wbc() > > __filemap_fdatawrite_range() > > file_write_and_wait_range() > > f2fs_do_sync_file() > > f2fs_sync_file() > > do_fsync() > > Freed by task stack: > > kfree() > > f2fs_stop_gc_thread() > > f2fs_do_shutdown() > > f2fs_shutdown() > > fs_bdev_mark_dead() > > > > Fixes: 5911d2d1d1a3 ("f2fs: introduce gc_merge mount option") > > Assisted-by: Codex:gpt-5.5 > > Signed-off-by: Zhang Cen > > --- > > Based on: > > - [PATCH] f2fs: embed f2fs_gc_kthread in f2fs_sb_info > > > > fs/f2fs/f2fs.h | 15 +++++++++++++++ > > fs/f2fs/gc.c | 22 +++++++++++++--------- > > fs/f2fs/segment.c | 16 ++++++++++------ > > fs/f2fs/super.c | 5 +++-- > > fs/f2fs/sysfs.c | 15 +++++++++------ > > 5 files changed, 50 insertions(+), 23 deletions(-) > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > > index 8011bbdf2c68..1aa8f8adddaa 100644 > > --- a/fs/f2fs/f2fs.h > > +++ b/fs/f2fs/f2fs.h > > @@ -19,6 +19,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -1750,6 +1751,7 @@ struct decompress_io_ctx { > > > > struct f2fs_gc_kthread { > > struct task_struct *f2fs_gc_task; > > + spinlock_t gc_task_lock; /* protects f2fs_gc_task */ > > wait_queue_head_t gc_wait_queue_head; > > > > /* for gc sleep time */ > > @@ -1775,6 +1777,19 @@ struct f2fs_gc_kthread { > > unsigned int boost_gc_greedy; > > }; > > > > +static inline struct task_struct * > > +f2fs_update_gc_task(struct f2fs_gc_kthread *gc_th, struct task_struct *task) > > +{ > > + struct task_struct *old_task; > > + > > + spin_lock(&gc_th->gc_task_lock); > > + old_task = READ_ONCE(gc_th->f2fs_gc_task); > > + WRITE_ONCE(gc_th->f2fs_gc_task, task); > > + spin_unlock(&gc_th->gc_task_lock); > > + > > + return old_task; > > +} > > + > > struct f2fs_sb_info { > > struct super_block *sb; /* pointer to VFS super block */ > > struct proc_dir_entry *s_proc; /* proc entry */ > > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c > > index e8244b92d8ce..7253a920153b 100644 > > --- a/fs/f2fs/gc.c > > +++ b/fs/f2fs/gc.c > > @@ -194,6 +194,7 @@ static int gc_thread_func(void *data) > > int f2fs_start_gc_thread(struct f2fs_sb_info *sbi) > > { > > struct f2fs_gc_kthread *gc_th = &sbi->gc_thread; > > + struct task_struct *task; > > dev_t dev = sbi->sb->s_bdev->bd_dev; > > > > gc_th->urgent_sleep_time = DEF_GC_THREAD_URGENT_SLEEP_TIME; > > @@ -219,26 +220,29 @@ int f2fs_start_gc_thread(struct f2fs_sb_info *sbi) > > > > init_waitqueue_head(&gc_th->gc_wait_queue_head); > > init_waitqueue_head(&gc_th->fggc_wq); > > - gc_th->f2fs_gc_task = kthread_run(gc_thread_func, sbi, > > - "f2fs_gc-%u:%u", MAJOR(dev), MINOR(dev)); > > - if (IS_ERR(gc_th->f2fs_gc_task)) { > > - int err = PTR_ERR(gc_th->f2fs_gc_task); > > + task = kthread_create(gc_thread_func, sbi, "f2fs_gc-%u:%u", > > + MAJOR(dev), MINOR(dev)); > > + if (IS_ERR(task)) { > > + int err = PTR_ERR(task); > > > > - gc_th->f2fs_gc_task = NULL; > > return err; > > } > > > > - set_user_nice(gc_th->f2fs_gc_task, > > - PRIO_TO_NICE(sbi->critical_task_priority)); > > + set_user_nice(task, PRIO_TO_NICE(sbi->critical_task_priority)); > > + f2fs_update_gc_task(gc_th, task); > > + wake_up_process(task); > > return 0; > > } > > > > void f2fs_stop_gc_thread(struct f2fs_sb_info *sbi) > > { > > struct f2fs_gc_kthread *gc_th = &sbi->gc_thread; > > + struct task_struct *task; > > > > - kthread_stop(gc_th->f2fs_gc_task); > > - gc_th->f2fs_gc_task = NULL; > > + task = f2fs_update_gc_task(gc_th, NULL); > > + if (!task) > > + return; > > + kthread_stop(task); > > wake_up_all(&gc_th->fggc_wq); > > } > > > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > > index 0b706568b034..e6e67c233e27 100644 > > --- a/fs/f2fs/segment.c > > +++ b/fs/f2fs/segment.c > > @@ -425,6 +425,8 @@ int f2fs_commit_atomic_write(struct inode *inode) > > */ > > void f2fs_balance_fs(struct f2fs_sb_info *sbi, bool need) > > { > > + struct f2fs_gc_kthread *gc_th = &sbi->gc_thread; > > + > > if (f2fs_cp_error(sbi)) > > return; > > > > @@ -452,14 +454,16 @@ void f2fs_balance_fs(struct f2fs_sb_info *sbi, bool need) > > f2fs_submit_merged_write(sbi, DATA); > > f2fs_submit_all_merged_ipu_writes(sbi); > > > > - if (test_opt(sbi, GC_MERGE) && sbi->gc_thread.f2fs_gc_task) { > > + if (test_opt(sbi, GC_MERGE) && READ_ONCE(gc_th->f2fs_gc_task)) { > > DEFINE_WAIT(wait); > > > > - prepare_to_wait(&sbi->gc_thread.fggc_wq, &wait, > > - TASK_UNINTERRUPTIBLE); > > - wake_up(&sbi->gc_thread.gc_wait_queue_head); > > - io_schedule(); > > - finish_wait(&sbi->gc_thread.fggc_wq, &wait); > > + prepare_to_wait(&gc_th->fggc_wq, &wait, > > + TASK_UNINTERRUPTIBLE); > > + if (READ_ONCE(gc_th->f2fs_gc_task)) { > > + wake_up(&gc_th->gc_wait_queue_head); > > + io_schedule(); > > + } > > + finish_wait(&gc_th->fggc_wq, &wait); > > } else { > > struct f2fs_gc_control gc_control = { > > .victim_segno = NULL_SEGNO, > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > > index e9ecd67a3f3b..140fffe93ea4 100644 > > --- a/fs/f2fs/super.c > > +++ b/fs/f2fs/super.c > > @@ -2943,11 +2943,11 @@ static int __f2fs_remount(struct fs_context *fc, struct super_block *sb) > > if ((flags & SB_RDONLY) || > > (F2FS_OPTION(sbi).bggc_mode == BGGC_MODE_OFF && > > !test_opt(sbi, GC_MERGE))) { > > - if (sbi->gc_thread.f2fs_gc_task) { > > + if (READ_ONCE(sbi->gc_thread.f2fs_gc_task)) { > > f2fs_stop_gc_thread(sbi); > > need_restart_gc = true; > > } > > - } else if (!sbi->gc_thread.f2fs_gc_task) { > > + } else if (!READ_ONCE(sbi->gc_thread.f2fs_gc_task)) { > > err = f2fs_start_gc_thread(sbi); > > if (err) > > goto restore_opts; > > @@ -5051,6 +5051,7 @@ static int f2fs_fill_super(struct super_block *sb, struct fs_context *fc) > > spin_lock_init(&sbi->inode_lock[i]); > > } > > mutex_init(&sbi->flush_lock); > > + spin_lock_init(&sbi->gc_thread.gc_task_lock); > > > > /* set a block size */ > > if (unlikely(!sb_set_blocksize(sb, F2FS_BLKSIZE))) { > > diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c > > index be92c05a5420..c83cd1d3ede2 100644 > > --- a/fs/f2fs/sysfs.c > > +++ b/fs/f2fs/sysfs.c > > @@ -664,7 +664,7 @@ static ssize_t __sbi_store(struct f2fs_attr *a, > > sbi->gc_mode = GC_NORMAL; > > } else if (t == 1) { > > sbi->gc_mode = GC_URGENT_HIGH; > > - if (sbi->gc_thread.f2fs_gc_task) { > > + if (READ_ONCE(sbi->gc_thread.f2fs_gc_task)) { > > sbi->gc_thread.gc_wake = true; > > wake_up_interruptible_all( > > &sbi->gc_thread.gc_wait_queue_head); > > @@ -674,7 +674,7 @@ static ssize_t __sbi_store(struct f2fs_attr *a, > > sbi->gc_mode = GC_URGENT_LOW; > > } else if (t == 3) { > > sbi->gc_mode = GC_URGENT_MID; > > - if (sbi->gc_thread.f2fs_gc_task) { > > + if (READ_ONCE(sbi->gc_thread.f2fs_gc_task)) { > > sbi->gc_thread.gc_wake = true; > > wake_up_interruptible_all( > > &sbi->gc_thread.gc_wait_queue_head); > > @@ -981,17 +981,20 @@ static ssize_t __sbi_store(struct f2fs_attr *a, > > } > > > > if (!strcmp(a->attr.name, "critical_task_priority")) { > > + int nice; > > + > > if (t < NICE_TO_PRIO(MIN_NICE) || t > NICE_TO_PRIO(MAX_NICE)) > > return -EINVAL; > > if (!capable(CAP_SYS_NICE)) > > return -EPERM; > > sbi->critical_task_priority = t; > > + nice = PRIO_TO_NICE(sbi->critical_task_priority); > > if (sbi->cprc_info.f2fs_issue_ckpt) > > - set_user_nice(sbi->cprc_info.f2fs_issue_ckpt, > > - PRIO_TO_NICE(sbi->critical_task_priority)); > > + set_user_nice(sbi->cprc_info.f2fs_issue_ckpt, nice); > > + spin_lock(&sbi->gc_thread.gc_task_lock); > > if (sbi->gc_thread.f2fs_gc_task) > > - set_user_nice(sbi->gc_thread.f2fs_gc_task, > > - PRIO_TO_NICE(sbi->critical_task_priority)); > > + set_user_nice(sbi->gc_thread.f2fs_gc_task, nice); > > + spin_unlock(&sbi->gc_thread.gc_task_lock); > > return count; > > } > > > > -- > > 2.43.0