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

[sashiko-bot@kernel.org]

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 <jinpu.wang@cloud.ionos.com>

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.

[ ... ]
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 87e17ba86d93a..0668a048db716 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
[ ... ]
> @@ -3119,7 +3119,7 @@ state_store(struct md_rdev *rdev, const char *buf, size_t len,
>  	} else if (cmd_match(buf, "remove")) {
>  		if (rdev->mddev->pers) {
>  			clear_bit(Blocked, &rdev->flags);
> -			remove_and_add_spares(rdev->mddev, rdev, NULL);
> +			remove_and_add_spares(rdev->mddev, rdev, lim);
>  		}
>  		if (rdev->raid_disk >= 0)
>  			err = -EBUSY;

[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.

[ ... ]
> @@ -3782,15 +3785,41 @@ rdev_attr_store(struct kobject *kobj, struct attribute *attr,
[ ... ]
>  	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.

[ ... ]
> @@ -8618,6 +8677,9 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
>  	    err != -EINVAL)
>  		mddev->hold_active = 0;
>  
> +	if (limp)
> +		err = queue_limits_commit_update(q, limp) ?: err;
> +

[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?

[ ... ]
> @@ -10419,6 +10501,8 @@ static void md_start_sync(struct work_struct *ws)
[ ... ]
>  	 *     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