Re: Re: [PATCH] btrfs: drain sysfs callbacks before stopping transaction kthread

["Jiacheng Xu" <stitch@zju.edu.cn>]

I think you are right. The previous patch only addressed the teardown
case and did not fix the mount-time window.

However, moving the whole btrfs_sysfs_add_mounted() call is not a
simple reorder because btrfs_init_space_info() creates child kobjects
under space_info_kobj, which is created by btrfs_sysfs_add_mounted().

Also, creating sysfs after transaction_kthread but before BTRFS_FS_OPEN
would still expose sysfs writes while the mount is not fully initialized.

Do you have any ideas?

Thanks,
Jiacheng

> -----原始邮件-----
> 发件人: "Qu Wenruo" <wqu@suse.com>
> 发送时间:2026-08-21 13:21:42 (星期五)
> 收件人: "Jiacheng Xu" <stitch@zju.edu.cn>
> 抄送: "Chris Mason" <clm@fb.com>, "David Sterba" <dsterba@suse.com>, linux-btrfs@vger.kernel.org, linux-kernel@vger.kernel.org
> 主题: Re: [PATCH] btrfs: drain sysfs callbacks before stopping transaction kthread
> 
> 
> 
> 在 2026/8/21 14:00, Jiacheng Xu 写道:
> > Hi Wenruo,
> > 
> > I agree that rejecting sysfs writes when FS_OPEN is unset or
> > CLOSING_START is set fixes the mount-time NULL pointer dereference.
> > 
> > However, checking these flags alone does not fully protect the teardown
> > path. There is still a check-then-use race:
> > 
> >        sysfs store callback                close_ctree()
> > 
> >        test FS_OPEN == 1
> >        test CLOSING_START == 0
> > 
> >                                             set CLOSING_START
> >                                             kthread_stop(transaction_kthread)
> > 
> >        wake_up_process(transaction_kthread)
> >        use a stopped or freed task_struct
> > 
> > Thus, the flag check fixes the reported initialization race, but a
> > separate teardown race remains unless active sysfs callbacks are drained
> > or otherwise synchronized before stopping transaction_kthread.
> 
> OK, then the next quesstion is, why we don't move the sysfs creation 
> after the commit transaction creation.
> 
> Even with your patch, it didn't solve the problem that during mount the 
> sysfs is created before transaction kthread.
> 
> So in theory it's possible to do sysfs write before kthread initialized, 
> still causing NULL pointer dereference.
> 
> > 
> > Thanks,
> > Jiacheng
> > 
> >> -----原始邮件-----
> >> 发件人: "Qu Wenruo" <wqu@suse.com>
> >> 发送时间:2026-08-21 06:32:14 (星期五)
> >> 收件人: "Jiacheng Xu" <stitch@zju.edu.cn>, "Chris Mason" <clm@fb.com>
> >> 抄送: "David Sterba" <dsterba@suse.com>, linux-btrfs@vger.kernel.org, linux-kernel@vger.kernel.org
> >> 主题: Re: [PATCH] btrfs: drain sysfs callbacks before stopping transaction kthread
> >>
> >>
> >>
> >> 在 2026/8/20 21:57, Jiacheng Xu 写道:
> >>> btrfs_label_store() and btrfs_feature_attr_store() wake up the
> >>> transaction kthread through fs_info->transaction_kthread.
> >>>
> >>> During filesystem teardown, close_ctree() stops the transaction kthread
> >>> before removing the mounted filesystem's sysfs attributes. A concurrent
> >>> sysfs write can therefore enter one of these callbacks after the kthread
> >>> has been stopped and pass an invalid task pointer to wake_up_process().
> >>>
> >>> This results in a concurrent null-pointer dereference in
> >>> try_to_wake_up(). The scheduler is not the root cause; the invalid
> >>> transaction kthread pointer is used by a Btrfs sysfs callback during
> >>> teardown.
> >>>
> >>> Split mounted sysfs cleanup into two stages. Remove attributes which may
> >>> have store callbacks before stopping the transaction kthread. The
> >>> remaining sysfs kobjects are removed at the original teardown point,
> >>> after the kthread has been stopped.
> >>
> >> Why not just simpliy reject sysfs write operations when the fs has
> >> CLOSING_START or without FS_OPEN flags?
> >>
> >>>
> >>> Apply the same ordering to the open_ctree() failure path when the
> >>> transaction kthread has already been created.
> >>>
> >>> Tested-by: Jiacheng Xu <stitch@zju.edu.cn>
> >>> Signed-off-by: Jiacheng Xu <stitch@zju.edu.cn>
> >>> ---
> >>> fs/btrfs/disk-io.c | 16 ++++++++++++++--
> >>> fs/btrfs/sysfs.c   | 26 +++++++++++++++++++++-----
> >>> fs/btrfs/sysfs.h   |  3 +++
> >>> 3 files changed, 38 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> >>> index 2f1666d9544e..4f5bcc576dc6 100644
> >>> --- a/fs/btrfs/disk-io.c
> >>> +++ b/fs/btrfs/disk-io.c
> >>> @@ -3363,6 +3363,7 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
> >>>         struct btrfs_root *tree_root;
> >>>         struct btrfs_root *chunk_root;
> >>>         struct btrfs_root *remap_root;
> >>> +     bool sysfs_attrs_removed = false;
> >>>         int ret;
> >>>         int level;
> >>>
> >>> @@ -3780,6 +3781,9 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
> >>> fail_qgroup:
> >>>         btrfs_free_qgroup_config(fs_info);
> >>> fail_trans_kthread:
> >>> +     btrfs_sysfs_remove_mounted_attrs(fs_info);
> >>> +     sysfs_attrs_removed = true;
> >>> +
> >>>         kthread_stop(fs_info->transaction_kthread);
> >>>         btrfs_cleanup_transaction(fs_info);
> >>>         btrfs_free_fs_roots(fs_info);
> >>> @@ -3793,7 +3797,9 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
> >>>         filemap_write_and_wait(fs_info->btree_inode->i_mapping);
> >>>
> >>> fail_sysfs:
> >>> -     btrfs_sysfs_remove_mounted(fs_info);
> >>> +     if (!sysfs_attrs_removed)
> >>> +             btrfs_sysfs_remove_mounted_attrs(fs_info);
> >>> +     btrfs_sysfs_remove_mounted_kobjects(fs_info);
> >>>
> >>> fail_fsdev_sysfs:
> >>>         btrfs_sysfs_remove_fsid(fs_info->fs_devices);
> >>> @@ -4318,6 +4324,9 @@ void __cold close_ctree(struct btrfs_fs_info *fs_info)
> >>>
> >>>         set_bit(BTRFS_FS_CLOSING_START, &fs_info->flags);
> >>>
> >>> +     /* Drain sysfs callbacks before stopping the transaction kthread. */
> >>> +     btrfs_sysfs_remove_mounted_attrs(fs_info);
> >>> +
> >>>         /*
> >>>         * If we had UNFINISHED_DROPS we could still be processing them, so
> >>>         * clear that bit and wake up relocation so it can stop.
> >>> @@ -4538,7 +4547,7 @@ void __cold close_ctree(struct btrfs_fs_info *fs_info)
> >>>                           percpu_counter_sum(&fs_info->ordered_bytes));
> >>>
> >>> -     btrfs_sysfs_remove_mounted(fs_info);
> >>> +     btrfs_sysfs_remove_mounted_kobjects(fs_info);
> >>>         btrfs_sysfs_remove_fsid(fs_info->fs_devices);
> >>>
> >>>         btrfs_put_block_group_cache(fs_info);
> >>>
> >>> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> >>> index 0d14570c8bc2..d90d76a152e9 100644
> >>> --- a/fs/btrfs/sysfs.c
> >>> +++ b/fs/btrfs/sysfs.c
> >>> @@ -1707,11 +1707,23 @@ static void btrfs_sysfs_remove_fs_devices(struct btrfs_fs_devices *fs_devices)
> >>>         }
> >>> }
> >>>
> >>> -void btrfs_sysfs_remove_mounted(struct btrfs_fs_info *fs_info)
> >>> +/*
> >>> + * Remove attributes which may have store callbacks. kernfs waits for active
> >>> + * callbacks during removal, so this must be done before stopping any kthread
> >>> + * which can be woken up by those callbacks.
> >>> + */
> >>> +void btrfs_sysfs_remove_mounted_attrs(struct btrfs_fs_info *fs_info)
> >>> {
> >>>         struct kobject *fsid_kobj = &fs_info->fs_devices->fsid_kobj;
> >>>
> >>> -     sysfs_remove_link(fsid_kobj, "bdi");
> >>> +     addrm_unknown_feature_attrs(fs_info, false);
> >>> +     sysfs_remove_group(fsid_kobj, &btrfs_feature_attr_group);
> >>> +     sysfs_remove_files(fsid_kobj, btrfs_attrs);
> >>> +}
> >>> +
> >>> +static void btrfs_sysfs_remove_mounted_dirs(struct btrfs_fs_info *fs_info)
> >>> +{
> >>> +     sysfs_remove_link(&fs_info->fs_devices->fsid_kobj, "bdi");
> >>>
> >>>         if (fs_info->space_info_kobj) {
> >>>               sysfs_remove_files(fs_info->space_info_kobj, allocation_attrs);
> >>> @@ -1730,9 +1742,18 @@ void btrfs_sysfs_remove_mounted(struct btrfs_fs_info *fs_info)
> >>>               kobject_put(fs_info->debug_kobj);
> >>>         }
> >>> #endif
> >>> -     addrm_unknown_feature_attrs(fs_info, false);
> >>> -     sysfs_remove_group(fsid_kobj, &btrfs_feature_attr_group);
> >>> -     sysfs_remove_files(fsid_kobj, btrfs_attrs);
> >>> +}
> >>> +
> >>> +void btrfs_sysfs_remove_mounted_kobjects(struct btrfs_fs_info *fs_info)
> >>> +{
> >>> +     btrfs_sysfs_remove_mounted_dirs(fs_info);
> >>> +     btrfs_sysfs_remove_fs_devices(fs_info->fs_devices);
> >>> +}
> >>> +
> >>> +void btrfs_sysfs_remove_mounted(struct btrfs_fs_info *fs_info)
> >>> +{
> >>> +     btrfs_sysfs_remove_mounted_dirs(fs_info);
> >>> +     btrfs_sysfs_remove_mounted_attrs(fs_info);
> >>>         btrfs_sysfs_remove_fs_devices(fs_info->fs_devices);
> >>> }
> >>>
> >>> diff --git a/fs/btrfs/sysfs.h b/fs/btrfs/sysfs.h
> >>> index 05498e5346c3..0d008fc8f1b8 100644
> >>> --- a/fs/btrfs/sysfs.h
> >>> +++ b/fs/btrfs/sysfs.h
> >>> @@ -35,6 +35,9 @@ void btrfs_kobject_uevent(struct block_device *bdev, enum kobject_action action)
> >>> int __init btrfs_init_sysfs(void);
> >>> void __cold btrfs_exit_sysfs(void);
> >>> int btrfs_sysfs_add_mounted(struct btrfs_fs_info *fs_info);
> >>> +void btrfs_sysfs_remove_mounted_attrs(struct btrfs_fs_info *fs_info);
> >>> +void btrfs_sysfs_remove_mounted_kobjects(struct btrfs_fs_info *fs_info);
> >>> void btrfs_sysfs_remove_mounted(struct btrfs_fs_info *fs_info);
> >>> void btrfs_sysfs_add_block_group_type(struct btrfs_block_group *cache);
> >>> int btrfs_sysfs_add_space_info_type(struct btrfs_space_info *space_info);