Re: [PATCH v2 5/8] md: take q->limits_lock before locking and suspending the array

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Missing rollback when `queue_limits_commit_update()` fails, leaving the array mutated with rejected and potentially dangerous block limits.

Pre-existing issues:
- [High] The patch leaves an ABBA deadlock unresolved in `action_store()` when triggering `ACTION_RESHAPE`.
- [High] Writing 'remove' to a spare disk's state attribute in a degraded array incorrectly hot-adds the disk to the array instead of removing it.
--

commit 7025b63183a90cd4ea9de7b41e69a4702b7c8d13
Author: Jack Wang [off-list ref]

md: take q->limits_lock before locking and suspending the array

This commit modifies block limit handling during MD array reconfigurations
to acquire q->limits_lock before locking and suspending the array. This 
fixes an ABBA deadlock between the limits lock, reconfig_mutex, and the 
array suspend state.

Two callers still take the lock inside reconfig_mutex with the array
suspended: ->start_reshape() from action_store(), which suspends before
flushing sync_work so the update cannot be held across it, and
raid*_run() from level_store(), which a later patch converts.

[Severity: High]
This is a pre-existing issue, but by leaving the action_store() path
unresolved here, does this leave the system vulnerable to the same ABBA
deadlock if a user writes 'reshape' to sync_action? Concurrent I/O could 
still park in the block layer, and a sysfs write to queue limits could 
still wait in blk_mq_freeze_queue for q_usage_counter to drain while 
mddev_stack_new_rdev attempts to acquire q->limits_lock.

[ ... ]

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but if an admin tries to remove
a spare from a degraded array by writing "remove" to the state attribute,
could this unintentionally hot-add the spare instead? Looking at
remove_and_add_spares(), rdev_removeable() would skip spares because they
have raid_disk < 0. Then rdev_addable() might return true, causing
hot_add_disk() to be called and returning -EBUSY.

[ ... ]

[ ... ]

 	rv = suspend ? mddev_suspend_and_lock(mddev) : mddev_lock(mddev);
 	if (!rv) {
 		if (rdev->mddev == NULL)
 			rv = -ENODEV;
 		else
-			rv = entry->store(rdev, page, length, NULL);
+			rv = entry->store(rdev, page, length, limp);
+		/* apply the limits before the array takes I/O again */
+		if (limp) {
+			int err = queue_limits_commit_update(q, limp);
+
+			limp = NULL;
+			if (err && rv >= 0)
+				rv = err;
+		}

[Severity: High]
If queue_limits_commit_update() fails and rejects the stacked limits, the
error is bubbled up, but is it possible that the array has already been 
mutated by entry->store() (such as structurally binding a new disk)? This
might leave the disk actively bound to the array configuration with
stale/rejected block limits.

[ ... ]

[Severity: High]
Similar to the rdev_attr_store() path above, if the limit commit fails
here, are the array mutations (such as ADD_NEW_DISK) reverted?

[ ... ]

[ ... ]

 	 *     https://bugzilla.kernel.org/show_bug.cgi?id=218200
 	 * Therefore, use __mddev_resume(mddev, false).
 	 */
+	if (limp && queue_limits_commit_update(q, limp))
+		pr_err("%s: could not apply queue limits\n", mdname(mddev));

[Severity: High]
If the limit commit fails, this merely logs an error. Will any spares 
recently added via remove_and_add_spares() remain fully active with
stale limits, potentially causing data corruption or crashing lower-level 
drivers?

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