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

[Qu Wenruo <wqu@suse.com>]

在 2026/8/21 16:38, Qu Wenruo 写道:
> 
> 
> 在 2026/8/21 16:19, Jiacheng Xu 写道:
>> 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?
> 
> For the space_info kobj, I think we can de-couple space info and its 
> kobj file creation.
> Aka, allow btrfs_init_space_info() to do everything except the kobj 
> creation.
> 
> Then at the very end, create every kobj needed, and at that time, the 
> full fs should be fully initialized.

Or we can de-couple only the btrfs_attrs files creation from 
btrfs_sysfs_add_mounted(), so that at early stages we only create needed 
sub-directories.

Then at the end of the mount, after everything is properly set up, 
create btrfs_attrs files.

For umount it's the reverse, remove btrfs_attrs files before anything else.


I think this is smaller than de-coupling space info.

> 
> This should solve the problem from the root, but will definitely need 
> quite some changes to the mount/unmount path.
>>
>> 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);
> 
>