Re: [PATCH v3 07/17] scsi-multipath: failover handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unchecked dereference of `sdev` returned by `scsi_device_from_queue()` leading to an immediate kernel panic.
- [High] Leaking a reference to `scsi_device` acquired by `scsi_device_from_queue()`.
- [High] Calling a sleepable synchronization function in `bio_end_io` atomic context.
--

commit 8a70735872bb7cb2125e8980a60e0bae2a38b040
Author: John Garry <john.g.garry@oracle.com>

scsi-multipath: failover handling

This commit implements failover handling for scsi commands that fail due to
the target device's transport going down. It requeues the master bio of each
bio attached to the request by handling the failover inside the
scsi_mpath_clone_end_io() completion handler.

> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> index 0f3f3f9fa5fae..f22e3677cf2ad 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c
[ ... ]
> @@ -242,11 +242,44 @@ static int scsi_multipath_sdev_init(struct scsi_device *sdev)
>  static void scsi_mpath_clone_end_io(struct bio *clone)
>  {
>  	struct bio *master_bio = clone->bi_private;
>  
>  	master_bio->bi_status = clone->bi_status;
> +
> +	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 = sdev->scsi_mpath_dev;

[Severity: High]
Can scsi_device_from_queue() return NULL here? If a path error occurs 
concurrently with a device removal or hot-unplug, get_device() inside 
scsi_device_from_queue() could fail. If sdev is NULL, wouldn't accessing 
sdev->scsi_mpath_dev on the very next line cause an immediate NULL pointer 
dereference?

> +		struct mpath_device *mpath_device = &scsi_mpath_dev->mpath_device;
> +		struct mpath_head *mpath_head = mpath_device->mpath_head;
> +		unsigned long flags;
> +
> +		scsi_mpath_dev_clear_path(scsi_mpath_dev);

[Severity: High]
Is it safe to call scsi_mpath_dev_clear_path() from a bio completion handler? 
Since scsi_mpath_clone_end_io() runs in atomic softirq context, and 
scsi_mpath_dev_clear_path() can call mpath_synchronize() which might sleep 
to wait for grace periods, could this trigger a sleeping in atomic context 
BUG?

> +
> +		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);
> +		return;

[Severity: High]
Does this return path leak the reference to the scsi_device? 

When scsi_device_from_queue() is called earlier, it increments the device's 
kobject reference count via get_device(). There doesn't appear to be a 
corresponding put_device() before returning here, which might permanently 
prevent device cleanup on unplug if repeated failovers occur.

> +	}
> +
>  	bio_put(clone);
>  	bio_endio(master_bio);
>  }

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