Re: [PATCH] md: make mddev_get() fail until md_alloc() has finished

[Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>]

Hi Kuai,

On Thu, Nov 12, 2026 at 12:13 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issues to consider:
> - [Medium] md: premature KOBJ_ADD uevent in add_disk() while mddev_get()
>   fails
> - [Low] md: stale reconfig_mutex lock and comment in md_alloc()
> It also noticed 1 related pre-existing issue this patch does not
> introduce:
> - [Medium] md: kernfs_node memory leak in mddev_unlock() [1]
> --
>
> commit 2ddaaecbff58458e8c38353164498300daf7d131
> Author: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
>
> md: make mddev_get() fail until md_alloc() has finished
>
> This commit adds an mddev->sysfs_live flag set under all_mddevs_lock after
> kobject_add() succeeds in md_alloc(), and updates mddev_get() to fail until
> that flag is set.
>
> Link: https://sashiko.dev/#/patchset/20261029202209.1422567-1-abd.masalkhi@gmail.com?part=4
>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 03e35386a8028..47118e9d1a656 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -641,7 +641,7 @@ static inline struct mddev *mddev_get(struct mddev *mddev)
>>  {
>>  	lockdep_assert_held(&all_mddevs_lock);
>>  
>> -	if (test_bit(MD_DELETED, &mddev->flags))
>> +	if (test_bit(MD_DELETED, &mddev->flags) || !mddev->sysfs_live)
>>  		return NULL;
>>  	atomic_inc(&mddev->active);
>>  	return mddev;
>> @@ -6432,10 +6432,16 @@ struct mddev *md_alloc(dev_t dev, char *name)
>>  		return ERR_PTR(error);
>>  	}
>>  
>> -	kobject_uevent(&mddev->kobj, KOBJ_ADD);
>>  	mddev->sysfs_state = sysfs_get_dirent_safe(mddev->kobj.sd, "array_state");
>>  	mddev->sysfs_level = sysfs_get_dirent_safe(mddev->kobj.sd, "level");
>
> [Severity: Medium]
> This problem wasn't introduced by this patch, but when
> legacy_async_del_gendisk is disabled, does mddev_unlock() leak
> mddev->sysfs_state and mddev->sysfs_level [1]?
>
> md_kobj_release() only calls sysfs_put() on mddev->sysfs_state and
> mddev->sysfs_level when legacy_async_del_gendisk is true, and the
> !legacy_async_del_gendisk teardown path in mddev_unlock() deletes
> mddev->kobj and mddev->gendisk without releasing those kernfs_node
> references:
>
> drivers/md/md.c:md_kobj_release() {
> 	if (legacy_async_del_gendisk) {
> 		if (mddev->sysfs_state)
> 			sysfs_put(mddev->sysfs_state);
> 		if (mddev->sysfs_level)
> 			sysfs_put(mddev->sysfs_level);
> 		del_gendisk(mddev->gendisk);
> 	}
> 	put_disk(mddev->gendisk);
> }
>
>> +
>> +	spin_lock(&all_mddevs_lock);
>> +	mddev->sysfs_live = true;
>> +	spin_unlock(&all_mddevs_lock);
>> +
>> +	kobject_uevent(&mddev->kobj, KOBJ_ADD);
>>  	mutex_unlock(&disks_mutex);
>> +
>>  	return mddev;
>>  
>>  out_put_disk:
>
> [Severity: Medium]
> Does making mddev_get() fail until mddev->sysfs_live is set cause a
> regression where udevd or concurrent openers fail with -ENODEV in md_open()
> and -EBUSY on md/ sysfs attributes right after add_disk() announces the
> disk?
>
> kobject_uevent(&mddev->kobj, KOBJ_ADD) is dropped by dev_uevent_filter()
> because mddev->kobj has ktype &md_ktype rather than &device_ktype.  The
> KOBJ_ADD uevent that actually reaches user space is sent earlier inside
> add_disk() -> add_disk_final() -> disk_uevent(disk, KOBJ_ADD), right after
> bdev_add() publishes the block device inode while mddev->sysfs_live is
> still false:
>
> CPU0                              CPU1 (udevd / opener)
> md_alloc()
>   add_disk(disk)
>     add_disk_final(disk)
>       bdev_add(...)
>       disk_uevent(disk, KOBJ_ADD)
>                                   blkdev_get_no_open()
>                                   md_open()
>                                     mddev_find()
>                                       mddev_get() -> NULL  <-- -ENODEV
>   kobject_add(&mddev->kobj, ...)
>   mddev->sysfs_live = true
>

I missed this case. Do you have a suggestion for a better fix, or should
I just drop this patch?

> [Severity: Low]
> Since mddev_get() now returns NULL until mddev->sysfs_live is set after
> mddev_unlock(mddev), md_open() can no longer succeed while reconfig_mutex
> is held across add_disk() and kobject_add() from commit d1472ca2ca5d ("md:
> don't hand out the array before md_alloc() has added mddev->kobj").
>
> drivers/md/md.c:md_alloc() {
> 	/*
> 	 * add_disk() makes the array openable before mddev->kobj exists.
> 	 * Hold reconfig_mutex until kobject_add() is done so that ioctls
> 	 * issued in between wait instead of using an uninitialised kobj.
> 	 */
> 	mddev_lock_nointr(mddev);
> 	error = add_disk(disk);
>     ...
> 	error = kobject_add(&mddev->kobj, &disk_to_dev(disk)->kobj, "%s", "md");
> 	mddev_unlock(mddev);
> }
>
> Does this make the comment and reconfig_mutex critical section in
> md_alloc() obsolete, as well as the commit message's first paragraph
> claiming that ioctls such as ADD_NEW_DISK can run before kobject_add()
> completes?
>
> [1] https://sashiko.dev/#/bug/linux-460ea5ed-96ea-49a0-8383-cfa84f28520b
>
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261112114958.2244-1-abd.masalkhi@gmail.com?part=1

-- 
Best Regards,
Abd-Alrhman