Re: [PATCH v2 6/8] md: pass a queue_limits through ->run()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Array starts successfully but fails to apply its queue limits if queue_limits_commit_update() returns an error, leaving a running array with inconsistent block limits.
--

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

md: pass a queue_limits through ->run()

This commit delegates the application of queue limits to the top-level callers
that start the array. This is done to prevent lock inversion between the
reconfig_mutex and q->limits_lock. Personalities now pass the limits up
to be committed by the caller.

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c

[ ... ]

> @@ -4826,6 +4880,9 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
>  			mddev->hold_active = 0;
>  		sysfs_notify_dirent_safe(mddev->sysfs_state);
>  	}
> +	/* apply the limits before the array takes I/O */
> +	if (limp)
> +		err = queue_limits_commit_update(q, limp) ?: err;
>  	mddev_unlock(mddev);

[Severity: High]
If queue_limits_commit_update() fails here, does it leave the array in an
inconsistent state?

The limits are committed after do_md_run() has already activated the array.
If the commit fails, an error is returned to userspace, but the running array
isn't stopped or rolled back. Can this lead to block layer warnings, driver
crashes, or data corruption if I/O requests exceed the capabilities of the
underlying devices?

[ ... ]

> @@ -8660,7 +8742,7 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
>  		goto unlock;
>  
>  	case RUN_ARRAY:
> -		err = do_md_run(mddev);
> +		err = do_md_run(mddev, limp);
>  		goto unlock;

[Severity: High]
Similarly, when do_md_run() successfully starts the array here, the limits
are committed later in the function (under the unlock label). If that commit
fails, the array will remain active with unvalidated or default limits.

Should there be a rollback mechanism (e.g., calling do_md_stop()) if the
limits cannot be successfully applied after the array is started?

> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c

[ ... ]

> @@ -3185,10 +3186,19 @@ static int raid1_set_limits(struct mddev *mddev)
>  	err = mddev_stack_rdev_limits(mddev, &lim, MDDEV_STACK_INTEGRITY);
>  	if (err)
>  		return err;
> +	/*
> +	 * The caller owns an update and commits it itself; taking
> +	 * q->limits_lock here would take it a second time.
> +	 */
> +	if (caller_lim) {
> +		*caller_lim = lim;
> +		return 0;
> +	}
> +
>  	return queue_limits_set(mddev->gendisk->queue, &lim);
>  }

[Severity: High]
By unconditionally returning 0 when caller_lim is provided, are we deferring
all limit validation until after the array is fully activated?

Because the personality's ->run() method copies the limits but does not
validate them with the block layer, the first point of failure becomes
queue_limits_commit_update() in the caller. Since the caller currently
lacks rollback handling, this guarantees the array is left running with
invalid limits if the configuration is rejected by blk_validate_limits().

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