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

[sashiko-bot@kernel.org]

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

[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