Re: [PATCH v6 15/27] scsi-multipath: clone each bio

On 29/07/2026 15:23, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] The `smpdt` structure fails to implement mandatory `mpath_head_template` callbacks, leading to a NULL pointer dereference.
> - [High] The clone completion callback entirely lacks the failover resubmission logic claimed in the commit message, breaking failover handling.
> - [High] The `bio_pool` mempool is destroyed before waiting for active users to drain, leading to a use-after-free when in-flight cloned bios complete.
> --
> 
> --- Patch [15]: [PATCH v6 15/27] scsi-multipath: clone each bio ---
> commit ea53159f3e5b8959582afd4b035267bb7a201a6d
> Author: John Garry <[email protected]>
> 
> scsi-multipath: clone each bio
> 
> For failover handling, we will take the approach to resubmit each
> bio.
> 
> However, unlike NVMe, for SCSI there is no guarantee that any bio submitted
> is either all or none completed.
> 
> As such, for SCSI, for failover handling we will take the approach to
> just re-submit the original bio. For this, clone and submit each bio.
> 
>> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
>> index 593f94aac70e9..0d439cac6447a 100644
>> --- a/drivers/scsi/scsi_multipath.c
>> +++ b/drivers/scsi/scsi_multipath.c
>> @@ -99,6 +99,7 @@ static void scsi_mpath_head_release(struct device *dev)
>>   		container_of(dev, struct scsi_mpath_head, dev);
>>   	struct mpath_head *mpath_head = &scsi_mpath_head->mpath_head;
>>   
>> +	bioset_exit(&scsi_mpath_head->bio_pool);
>>   	ida_free(&scsi_multipath_dev_ida, scsi_mpath_head->index);
>>   	ida_destroy(&scsi_mpath_head->ida);
>>   	mpath_head_uninit(mpath_head);
> 
> [Severity: High]
> Does this code destroy the mempool before all users have drained?

Yes, all bios should be finished at this stage

> 
> When the last SCSI path is removed, scsi_mpath_head_release() calls
> bioset_exit() immediately. It then calls mpath_head_uninit(), which
> synchronously waits for all users to release their references.
> 
> If there are in-flight cloned bios completing, or if the block device is
> still open and receives new I/O while waiting, they could access the
> already-freed bio_pool via bio_put() or bio_alloc_clone().
> 
> Can this result in a use-after-free on the destroyed mempool?
> 
> [ ... ]
> 
>> @@ -237,7 +238,34 @@ static int scsi_multipath_sdev_init(struct scsi_device *sdev)
>>   	return 0;
>>   }
>>   
>> +static void scsi_mpath_clone_end_io(struct bio *clone)
>> +{
>> +	struct bio *master_bio = clone->bi_private;
>> +
>> +	master_bio->bi_status = clone->bi_status;
>> +	bio_put(clone);
>> +	bio_endio(master_bio);
>> +}
> 
> [Severity: High]
> Does this callback omit the failover resubmission logic mentioned in the
> commit message?
> 
> When an underlying SCSI path fails an I/O, this function unconditionally
> copies the error to master_bio and calls bio_endio().
> 
> Since the commit message states the approach is to re-submit the original
> bio for failover handling, shouldn't there be code here to intercept the
> error and requeue it, rather than immediately returning the error to the
> issuer?
> 

this is done in a later patch

> [ ... ]
> 
>>   static struct mpath_head_template smpdt = {
>> +	.clone_bio = scsi_mpath_clone_bio,
>>   };
> 
> [Severity: High]
> Is this struct missing mandatory mpath_head_template callbacks?
> 
> Any I/O submission to the multipath block device invokes mpath_find_path(),
> which unconditionally calls mpath_head->mpdt->is_disabled(mpath_device).
> 
> Because smpdt only initializes .clone_bio, functions like is_disabled,
> is_optimized, and available_path will be NULL.
> 
> Will this cause an immediate kernel panic due to a NULL pointer dereference
> when an I/O is issued?
> 

they are added later