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

[Johannes Thumshirn]

On 10/13/26 1:13 PM, Daniel Vacek wrote:
> On Wed, 7 Oct 2026 at 18:04, Adarsh Das <adarshdas950@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Link: https://lore.kernel.org/all/6a791c94.01d0871a.3a0d52.0099.GAE@xxxxxxxxxx
> > Tested-by: syzbot+3bf3e110b2d406b8166c@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Assisted-by: LLM
> > Signed-off-by: Adarsh Das <adarshdas950@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxx/
> > ---
> >   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.