Re: [PATCH v2 7/8] md: open new legs before locking the array
From: sashiko-bot@kernel.org
Date: 2026-09-10 08:48:16
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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