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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] queue_limits_commit_update() return value is ignored and called unconditionally on error paths instead of queue_limits_cancel_update()

Pre-existing issues:
- [High] ABBA deadlock between q->limits_lock and mddev_suspend in level_store()
--

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

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

This commit updates the block layer limits locking order for MD devices to
prevent a deadlock when writing queue limits attributes during disk operations.
It takes the limits update before locking and suspending the array, passing it
down into personality stacks.

> Two callers still take the lock inside reconfig_mutex, both 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() -> queue_limits_set() from level_store(), which already
> hangs on its own because it freezes the queue while suspended.

[Severity: High]
This is a pre-existing issue, but is there a plan to address the ABBA
deadlock in level_store()? When the array is suspended and locked, normal
I/O parks on the queue usage counter. Concurrently, a udev sysfs write can
take q->limits_lock and wait in blk_mq_freeze_queue() for the counter to
drain, while level_store() holds the suspend lock and waits for
q->limits_lock via pers->run() -> queue_limits_set(), causing a system
hang.

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -3780,15 +3783,37 @@ 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) {
> +			queue_limits_commit_update(q, limp);
> +			limp = NULL;
> +		}

[Severity: Medium]
Are we missing a check for the return value of queue_limits_commit_update()
here? If the block layer rejects the new limits because blk_validate_limits()
fails, the return value is ignored. This could allow the array to proceed
using the new disk with old limits, risking I/O errors if requests exceed
the disk's hardware limits.

>  		suspend ? mddev_unlock_and_resume(mddev) : mddev_unlock(mddev);
>  	}
>  
> +	if (limp)
> +		queue_limits_commit_update(q, limp);
> +

[Severity: Medium]
Does this code incorrectly call queue_limits_commit_update() unconditionally
on error paths? If mddev_suspend_and_lock() or mddev_lock() failed, rv will
be non-zero but limp will still be set. In this situation, the code will
commit unchanged limits instead of cancelling them via
queue_limits_cancel_update().

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

[Severity: Medium]
Similar to the rdev_attr_store() issue above, should this use
queue_limits_cancel_update() on error paths? If an inner ioctl command
fails, this will erroneously commit the unchanged limits rather than
cancelling the update.

[ ... ]
> @@ -10419,6 +10497,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);

[Severity: Medium]
Does this safely handle limit validation failures when adding spares? The
return value is ignored, which could leave the array with outdated limits if
the limits fail to stack correctly.

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