Re: [PATCH v7 18/27] scsi-multipath: failover handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Path failover is prematurely aborted if the underlying path device is removed, and dropping its reference causes a sleep-in-atomic bug.
--

commit 657b83f2533d51dce7950c14bb5248625186e04d
Author: John Garry <john.garry@linux.dev>
Subject: scsi-multipath: failover handling

This commit implements failover handling for scsi_cmnd by requeuing the
master bio of each bio attached to a failed request. Failover is detected
in scsi_mpath_clone_end_io() when blk_path_error() occurs.

> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c

[ ... ]

>  static void scsi_mpath_clone_end_io(struct bio *clone)
>  {
>  	struct bio *master_bio = clone->bi_private;
>  
> +	if (clone->bi_status && blk_path_error(clone->bi_status)) {
> +		struct block_device *bi_bdev = clone->bi_bdev;
> +		struct request_queue *q = bi_bdev->bd_queue;
> +		struct scsi_device *sdev = scsi_device_from_queue(q);
> +		struct scsi_mpath_device *scsi_mpath_dev;
> +		struct mpath_device *mpath_device;
> +		struct mpath_head *mpath_head;
> +		unsigned long flags;
> +
> +		if (!sdev)
> +			goto end_bio;

[Severity: High]
If the underlying device is being removed and scsi_device_from_queue()
returns NULL, does this prematurely abort the failover?

Bailing out here directly propagates the path error to the master bio,
which seems to break the multipath failover guarantee.

> +
> +		scsi_mpath_dev = sdev->scsi_mpath_dev;
> +		mpath_device = &scsi_mpath_dev->mpath_device;
> +		mpath_head = mpath_device->mpath_head;
> +
> +		spin_lock_irqsave(&mpath_head->requeue_lock, flags);
> +		bio_list_add_clone(&mpath_head->requeue_list, clone);
> +		spin_unlock_irqrestore(&mpath_head->requeue_lock, flags);
> +
> +		mpath_schedule_requeue_work(mpath_head);
> +		put_device(&sdev->sdev_gendev);

[Severity: High]
Can this put_device() call trigger a sleep-in-atomic panic?

Since scsi_mpath_clone_end_io() executes in the softirq context of a bio
end_io handler, dropping the last reference here can call
scsi_device_dev_release(), which explicitly enforces might_sleep().

Could mpath_head be safely derived directly from
master_bio->bi_bdev->bd_disk->private_data instead of indirectly through
the underlying path device?

> +		return;
> +	}
> +
> +end_bio:
>  	master_bio->bi_status = clone->bi_status;
>  	bio_put(clone);
>  	bio_endio(master_bio);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901104526.3850621-1-john.g.garry@oracle.com?part=18