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

[John Garry <john.g.garry@oracle.com>]

On 03/07/2026 12:40, sashiko-bot@kernel.org wrote:
> 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?

will fix

> 
>> +		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?

I'd like more info for when this is called from softirq context. From my 
experience we the failover completion call from blk_mq_work_fn in ... 
scsi_queue_rq() -> blk_update_request()

> 
>> +
>> +		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.

will fix

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