Re: [PATCH 2/6] md: pass a queue_limits down to ->hot_add_disk()

[sashiko-bot@kernel.org]

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

New issues:
- [High] `linear_add()` ignores the new `lim` argument and unconditionally calls `queue_limits_set()`, which will cause a deadlock.

Pre-existing issues:
- [High] Missing limit check and queue limit stacking for replacement disks in `raid1_add_disk()`.
- [High] Missing limits update and stacking when hot-adding a disk in `raid5_add_disk()`.
--

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

    md: pass a queue_limits down to ->hot_add_disk()
    
    Adding a leg stacks its queue limits, which mddev_stack_new_rdev() does
    by taking q->limits_lock itself.  Callers that hold reconfig_mutex, or
    have the array suspended, must not do that: the lock's holder waits for
    I/O they are blocking.  They need to own the update instead.
    
    Give ->hot_add_disk(), remove_and_add_spares() and
    md_choose_sync_action() a queue_limits argument, and add
    mddev_stack_rdev_into() to stack into a caller-owned update.  Every
    caller passes NULL, so no functional change; the users follow.

> diff --git a/drivers/md/md-linear.c b/drivers/md/md-linear.c
> index 73b367b61b873..1f44c2d7db93d 100644
> --- a/drivers/md/md-linear.c
> +++ b/drivers/md/md-linear.c
> @@ -186,7 +186,8 @@ static int linear_run(struct mddev *mddev)
>  	return ret;
>  }
>  
> -static int linear_add(struct mddev *mddev, struct md_rdev *rdev)
> +static int linear_add(struct mddev *mddev, struct md_rdev *rdev,
> +		      struct queue_limits *lim)
>  {
>  	/* Adding a drive to a linear array allows the array to grow.
>  	 * It is permitted if the new drive has a matching superblock

[Severity: High]
Does linear_add() miss using the new lim argument?

Looking at the body of linear_add(), it unconditionally calls linear_conf() 
without passing the limits down. In the final state of this patch series, when 
the caller already holds q->limits_lock, linear_conf() will eventually call 
queue_limits_set() which tries to take the lock again. Could this lead to a 
deadlock on q->limits_lock when hot-adding a disk?

[ ... ]
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index f0646fb24371f..dd348b5695d06 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1898,7 +1898,8 @@ static bool raid1_remove_conf(struct r1conf *conf, int disk)
>  	return true;
>  }
>  
> -static int raid1_add_disk(struct mddev *mddev, struct md_rdev *rdev)
> +static int raid1_add_disk(struct mddev *mddev, struct md_rdev *rdev,
> +			  struct queue_limits *lim)
>  {
>  	struct r1conf *conf = mddev->private;
>  	int err = -EEXIST;
> @@ -1923,7 +1924,10 @@ static int raid1_add_disk(struct mddev *mddev, struct md_rdev *rdev)
>  	for (mirror = first; mirror <= last; mirror++) {
>  		p = conf->mirrors + mirror;
>  		if (!p->rdev) {
> -			err = mddev_stack_new_rdev(mddev, rdev);
> +			if (lim)
> +				err = mddev_stack_rdev_into(mddev, rdev, lim);
> +			else
> +				err = mddev_stack_new_rdev(mddev, rdev);
>  			if (err)
>  				return err;

[Severity: High]
This is a pre-existing issue, but I noticed that further down in 
raid1_add_disk(), we add a replacement disk but omit any limit checks:

	if (err && repl_slot >= 0) {
		/* Add this device as a replacement */
		clear_bit(In_sync, &rdev->flags);
		set_bit(Replacement, &rdev->flags);
		raid1_add_conf(conf, rdev, repl_slot, true);
		err = 0;
		conf->fullsync = 1;
	}

Since mddev_stack_new_rdev() or mddev_stack_rdev_into() is skipped for this 
path, could this allow a replacement disk with incompatible queue limits 
(such as differing integrity profiles or larger logical block sizes) to join 
the array, resulting in data corruption or I/O failures?

[ ... ]
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index b91545ce090de..0ec555ada64a0 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -8441,7 +8441,8 @@ static int raid5_remove_disk(struct mddev *mddev, struct md_rdev *rdev)
>  	return err;
>  }
>  
> -static int raid5_add_disk(struct mddev *mddev, struct md_rdev *rdev)
> +static int raid5_add_disk(struct mddev *mddev, struct md_rdev *rdev,
> +			  struct queue_limits *lim)
>  {
>  	struct r5conf *conf = mddev->private;
>  	int ret, err = -EEXIST;

[Severity: High]
This isn't a bug introduced by this patch, but I noticed that raid5_add_disk()
does not stack or check queue limits at all when hot-adding a disk:

	for (disk = first; disk <= last; disk++) {
		p = conf->disks + disk;
		if (p->rdev == NULL) {
			clear_bit(In_sync, &rdev->flags);
			rdev->raid_disk = disk;
			if (rdev->saved_raid_disk != disk)
				conf->fullsync = 1;
			WRITE_ONCE(p->rdev, rdev);

			err = log_modify(conf, rdev, true);

			goto out;
		}
	}

Because mddev_stack_new_rdev() or mddev_stack_rdev_into() are not called
here, and the new lim argument is ignored, could this allow disks with
incompatible limits to be successfully added to the array?

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