Re: [PATCH v2] block: fix kobject double initialization in add_disk

[Nilay Shroff]

On 8/8/25 12:53 PM, Yu Kuai wrote:
> Hi,
> 
> 在 2025/08/08 15:15, Nilay Shroff 写道:
>>
>>
>> On 8/8/25 11:06 AM, Zheng Qixing wrote:
>>> From: Zheng Qixing <zhengqixing@xxxxxxxxxx>
>>>
>>> Device-mapper can call add_disk() multiple times for the same gendisk
>>> due to its two-phase creation process (dm create + dm load). This leads
>>> to kobject double initialization errors when the underlying iSCSI devices
>>> become temporarily unavailable and then reappear.
>>>
>>> However, if the first add_disk() call fails and is retried, the queue_kobj
>>> gets initialized twice, causing:
>>>
>>> kobject: kobject (ffff88810c27bb90): tried to init an initialized object,
>>> something is seriously wrong.
>>>   Call Trace:
>>>    <TASK>
>>>    dump_stack_lvl+0x5b/0x80
>>>    kobject_init.cold+0x43/0x51
>>>    blk_register_queue+0x46/0x280
>>>    add_disk_fwnode+0xb5/0x280
>>>    dm_setup_md_queue+0x194/0x1c0
>>>    table_load+0x297/0x2d0
>>>    ctl_ioctl+0x2a2/0x480
>>>    dm_ctl_ioctl+0xe/0x20
>>>    __x64_sys_ioctl+0xc7/0x110
>>>    do_syscall_64+0x72/0x390
>>>    entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>>
>>> Fix this by separating kobject initialization from sysfs registration:
>>>   - Initialize queue_kobj early during gendisk allocation
>>>   - add_disk() only adds the already-initialized kobject to sysfs
>>>   - del_gendisk() removes from sysfs but doesn't destroy the kobject
>>>   - Final cleanup happens when the disk is released
>>>
>>> Fixes: 2bd85221a625 ("block: untangle request_queue refcounting from sysfs")
>>> Reported-by: Li Lingfeng <lilingfeng3@xxxxxxxxxx>
>>> Closes: https://lore.kernel.org/all/83591d0b-2467-433c-bce0-5581298eb161@xxxxxxxxxx/
>>> Signed-off-by: Zheng Qixing <zhengqixing@xxxxxxxxxx>
>>> ---
>>>   block/blk-sysfs.c | 12 +++++-------
>>>   block/blk.h       |  1 +
>>>   block/genhd.c     |  2 ++
>>>   3 files changed, 8 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
>>> index 396cded255ea..c5cf79a20842 100644
>>> --- a/block/blk-sysfs.c
>>> +++ b/block/blk-sysfs.c
>>> @@ -847,7 +847,7 @@ static void blk_queue_release(struct kobject *kobj)
>>>       /* nothing to do here, all data is associated with the parent gendisk */
>>>   }
>>>   -static const struct kobj_type blk_queue_ktype = {
>>> +const struct kobj_type blk_queue_ktype = {
>>>       .default_groups = blk_queue_attr_groups,
>>>       .sysfs_ops    = &queue_sysfs_ops,
>>>       .release    = blk_queue_release,
>>> @@ -875,15 +875,14 @@ int blk_register_queue(struct gendisk *disk)
>>>       struct request_queue *q = disk->queue;
>>>       int ret;
>>>   -    kobject_init(&disk->queue_kobj, &blk_queue_ktype);
>>>       ret = kobject_add(&disk->queue_kobj, &disk_to_dev(disk)->kobj, "queue");
>>>       if (ret < 0)
>>> -        goto out_put_queue_kobj;
>>> +        return ret;
>>>         if (queue_is_mq(q)) {
>>>           ret = blk_mq_sysfs_register(disk);
>>>           if (ret)
>>> -            goto out_put_queue_kobj;
>>> +            goto out_del_queue_kobj;
>>>       }
>>>       mutex_lock(&q->sysfs_lock);
>>>   @@ -934,8 +933,8 @@ int blk_register_queue(struct gendisk *disk)
>>>       mutex_unlock(&q->sysfs_lock);
>>>       if (queue_is_mq(q))
>>>           blk_mq_sysfs_unregister(disk);
>>> -out_put_queue_kobj:
>>> -    kobject_put(&disk->queue_kobj);
>>> +out_del_queue_kobj:
>>> +    kobject_del(&disk->queue_kobj);
>>>       return ret;
>>>   }
>>>   @@ -986,5 +985,4 @@ void blk_unregister_queue(struct gendisk *disk)
>>>           elevator_set_none(q);
>>>         blk_debugfs_remove(disk);
>>> -    kobject_put(&disk->queue_kobj);
>>>   }
>> Shouldn't we replace kobject_put() with kobject_del() here in
>> blk_unregister_queue()?
> 
> Looks like you missed that kobject_del() is called before the
> kobject_put().
> 
>         /* Now that we've deleted all child objects, we can delete the queue. */
>         kobject_uevent(&disk->queue_kobj, KOBJ_REMOVE);
>         kobject_del(&disk->queue_kobj);
> 
>         if (queue_is_mq(q))
>                 elevator_set_none(q);
> 
>         blk_debugfs_remove(disk);
>         kobject_put(&disk->queue_kobj);
> 
>>
Oh yes I missed to notice it since that was not part of the
patch. Thanks! 

This patch now looks good to me:
Reviewed-by: Nilay Shroff <nilay@xxxxxxxxxxxxx>