Re: [PATCH v2] btrfs: unwind device add when sysfs registration fails

[Johannes Thumshirn <johannes.thumshirn@wdc.com>]

On 10/13/26 1:13 PM, Daniel Vacek wrote:
> On Wed, 7 Oct 2026 at 18:04, Adarsh Das <adarshdas950@gmail.com> wrote:
>> btrfs_init_new_device() ignored the return value of
>> btrfs_sysfs_add_device(). When sysfs link creation failed (e.g. -EEXIST),
>> the add path continued, aborted the transaction with the same errno, and
>> tripped WARN_ON(btrfs_abort_should_print_stack()) while leaving the device
>> half-registered in memory.
>>
>> Check the return value and unwind when registration fails, without
>> aborting the filesystem for errnos like -EEXIST. If add failed before
>> anything was published in sysfs, tear down the in-memory device state only
>> and do not call btrfs_sysfs_remove_device(). When the block-device link
>> was created but devid kobject registration fails, drop the link inside
>> btrfs_sysfs_add_device() before returning the error.
>>
>> Reported-by: syzbot+3bf3e110b2d406b8166c@syzkaller.appspotmail.com
>> Link: https://lore.kernel.org/all/6a791c94.01d0871a.3a0d52.0099.GAE@google.com
>> Tested-by: syzbot+3bf3e110b2d406b8166c@syzkaller.appspotmail.com
>> Assisted-by: LLM
>> Signed-off-by: Adarsh Das <adarshdas950@gmail.com>
>> ---
>> v2:
>>   - Unwind without sysfs remove when add failed (review on v1)
>>   - Roll back block-device link if devid kobject add fails
>> v1: https://lore.kernel.org/all/20261003111951.24527-1-adarshdas950@gmail.com/
>> ---
>>   fs/btrfs/sysfs.c   | 3 +++
>>   fs/btrfs/volumes.c | 7 ++++++-
>>   2 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
>> index 39cb01ee441a..2f034689e71b 100644
>> --- a/fs/btrfs/sysfs.c
>> +++ b/fs/btrfs/sysfs.c
>> @@ -2165,6 +2165,9 @@ int btrfs_sysfs_add_device(struct btrfs_device *device)
>>          ret = kobject_init_and_add(&device->devid_kobj, &devid_ktype,
>>                                     devinfo_kobj, "%llu", device->devid);
>>          if (ret) {
>> +               if (device->bdev)
>> +                       sysfs_remove_link(devices_kobj,
>> +                                         bdev_kobj(device->bdev)->name);
>>                  kobject_put(&device->devid_kobj);
>>                  btrfs_warn(device->fs_info,
>>                             "devinfo init for devid %llu failed: %d",
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 85ea9c5d4536..56b38f012093 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -3066,7 +3066,11 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
>>          mutex_unlock(&fs_info->chunk_mutex);
>>
>>          /* Add sysfs device entry */
>> -       btrfs_sysfs_add_device(device);
>> +       ret = btrfs_sysfs_add_device(device);
>> +       if (ret) {
>> +               mutex_unlock(&fs_devices->device_list_mutex);
>> +               goto error_unwind_device;
>> +       }
>>
>>          mutex_unlock(&fs_devices->device_list_mutex);
>>
>> @@ -3147,6 +3151,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
>>
>>   error_sysfs:
>>          btrfs_sysfs_remove_device(device);
>> +error_unwind_device:
>>          mutex_lock(&fs_info->fs_devices->device_list_mutex);
>>          if (seeding_dev)
>>                  btrfs_assign_next_active_device(device, seed_devices->latest_dev);
> I'm wondering why not this?:
>
> @@ -3066,7 +3066,9 @@ int btrfs_init_new_device(struct btrfs_fs_info
> *fs_info, const char *device_path
>          mutex_unlock(&fs_info->chunk_mutex);
>
>          /* Add sysfs device entry */
> -       btrfs_sysfs_add_device(device);
> +       ret = btrfs_sysfs_add_device(device);
> +       if (ret)
> +               goto error_device;
>
>          mutex_unlock(&fs_devices->device_list_mutex);
>
> @@ -3148,6 +3150,7 @@ int btrfs_init_new_device(struct btrfs_fs_info
> *fs_info, const char *device_path
>   error_sysfs:
>          btrfs_sysfs_remove_device(device);
>          mutex_lock(&fs_info->fs_devices->device_list_mutex);
> +error_device:
>          mutex_lock(&fs_info->chunk_mutex);
>          list_del_rcu(&device->dev_list);
>          list_del(&device->dev_alloc_list);
>
> --nX

This would remove the need for the lock of the device_list_mutex indeed.