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

[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.

Thanks,
Jiacheng

> -----原始邮件-----
> 发件人: "Qu Wenruo" <wqu@xxxxxxxx>
> 发送时间:2026-08-21 06:32:14 (星期五)
> 收件人: "Jiacheng Xu" <stitch@xxxxxxxxxx>, "Chris Mason" <clm@xxxxxx>
> 抄送: "David Sterba" <dsterba@xxxxxxxx>, linux-btrfs@xxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx
> 主题: 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@xxxxxxxxxx>
> > Signed-off-by: Jiacheng Xu <stitch@xxxxxxxxxx>
> > ---
> > 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);