Re: [PATCH v2 7/8] md: open new legs before locking the array

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The patch leaves export_rdev() calls inside the mddev->reconfig_mutex critical section on error paths, failing to completely break the disk->open_mutex deadlock cycle.
- [Medium] The snapshot check in md_add_new_disk() is bypassed when nd->rdev is NULL, allowing concurrent array reconfigurations to trigger a WARN_ON_ONCE.
--

commit 140222a8600d2514747d88643188f131c16b6c50
Author: Jack Wang <jinpu.wang@cloud.ionos.com>

md: open new legs before locking the array

This commit aims to resolve a lockdep cycle between disk->open_mutex and
mddev->reconfig_mutex by opening new devices before acquiring the array
lock. It updates functions like md_import_new_disk() and new_dev_store()
to snapshot configuration and validate it after locking.

> diff --git a/drivers/md/md-autodetect.c b/drivers/md/md-autodetect.c
> index e15ae2fb58a2d..e592577356ad3 100644
> --- a/drivers/md/md-autodetect.c
> +++ b/drivers/md/md-autodetect.c
> @@ -216,7 +217,21 @@ static void __init md_setup_drive(struct md_setup_args *args)
>  				(1 << MD_DISK_ACTIVE) | (1 << MD_DISK_SYNC);
>  		}
>  
> -		md_add_new_disk(mddev, &dinfo, NULL);
> +		/*
> +		 * Opening a leg takes disk->open_mutex, which must not nest
> +		 * inside reconfig_mutex, see md_import_new_disk().  Drop the
> +		 * array lock around it; this is __init and the array is not
> +		 * reachable yet, so nothing else can touch it in between.
> +		 */
> +		mddev_unlock(mddev);
> +		if (md_import_new_disk(mddev, &dinfo, &nd)) {
> +			mddev_lock_nointr(mddev);
> +			continue;
> +		}
> +		mddev_lock_nointr(mddev);
> +
> +		md_add_new_disk(mddev, &dinfo, &nd, NULL);
> +		md_put_new_disk(&nd);

[Severity: High]
Does md_put_new_disk() recreate the lockdep cycle we are trying to fix?

Since md_put_new_disk() calls export_rdev() internally, and export_rdev()
acquires disk->open_mutex, calling it here while mddev_lock_nointr() is
held appears to preserve the reconfig_mutex -> disk->open_mutex lock order.

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 5be956e80563d..fa033d7d38318 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c

[ ... ]

> @@ -7685,12 +7706,35 @@ static int get_disk_info(struct mddev *mddev, void __user * arg)
>  	return 0;
>  }
>  
> +/*
> + * @nd carries an rdev the caller opened before locking the array, for the
> + * branch its snapshot selected.  Every caller must open first; doing it
> + * here would nest disk->open_mutex inside reconfig_mutex.
> + */
>  int md_add_new_disk(struct mddev *mddev, struct mdu_disk_info_s *info,
> -		    struct queue_limits *lim)
> +		    struct md_new_disk *nd, struct queue_limits *lim)
>  {
>  	struct md_rdev *rdev;
>  	dev_t dev = MKDEV(info->major,info->minor);
>  
> +	/*
> +	 * The open ran unlocked, so anything that selects a different branch
> +	 * below, or a different superblock format, means it was done against
> +	 * an array that no longer looks like this one.
> +	 */
> +	if (nd && nd->rdev &&
> +	    (nd->have_raid_disks != (mddev->raid_disks != 0) ||
> +	     nd->have_pers != !!mddev->pers ||
> +	     nd->persistent != mddev->persistent ||
> +	     nd->major_version != mddev->major_version ||
> +	     nd->minor_version != mddev->minor_version)) {

[Severity: Medium]
Is this validation check bypassed if nd->rdev is NULL?

If md_import_new_disk() is called on an array where ADD_NEW_DISK is
unsupported (for example, raid_disks != 0 and pers == NULL), it returns 0
with nd->rdev = NULL. If a concurrent ioctl like STOP_ARRAY resets
raid_disks to 0 before md_add_new_disk() acquires the lock, the condition
checking nd->rdev evaluates to false, skipping the validation entirely.

This allows the function to proceed into the !mddev->raid_disks branch
and hit the WARN_ON_ONCE(!nd || !nd->rdev) below, triggering a kernel splat.

> +		pr_warn("%s: array reconfigured while opening %pg\n",
> +			mdname(mddev), nd->rdev->bdev);
> +		export_rdev(nd->rdev);
> +		nd->rdev = NULL;
> +		return -EBUSY;
> +	}

[Severity: High]
This new failure path also executes with reconfig_mutex held, directly
calling export_rdev() and recreating the lockdep cycle.

Could this leave the system vulnerable to the same deadlock on error paths?

This pattern also occurs in new_dev_store(), where the error path calls
export_rdev() before mddev_unlock_and_resume().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910081114.1605746-1-jinpu.wang@ionos.com?part=7