Re: [PATCH v5 20/27] scsi-multipath: add scsi_mpath_{start,end}_request()

On 27/07/2026 16:25, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] Synchronous dispatch errors and request cancellations bypass `scsi_mpath_end_request()`, permanently leaking resources and causing NULL pointer dereferences on recycled tags.
> - [Critical] The dummy implementation of `scsi_mpath_end_request()` skips `blk_update_request()`, silently dropping bio completions if CONFIG_SCSI_MULTIPATH is disabled.
> --
> 
> --- Patch [20]: [PATCH v5 20/27] scsi-multipath: add scsi_mpath_{start,end}_request() ---
> commit 375ed243ce77c97b0b6e6eb569248a68ab42aec6
> Author: John Garry <[email protected]>
> scsi-multipath: add scsi_mpath_{start,end}_request()
> 
> This commit introduces scsi_mpath_start_request() and scsi_mpath_end_request()
> to update private multipath request data. It also adds mpath_nr_active to
> Scsi_Host to support queue depth multipath iopolicies.
> 
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index 6a2d7dbfbe8c3..36e5c5543754f 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -669,7 +669,10 @@ static bool scsi_end_request(struct request *req, blk_status_t error,
>>   	struct scsi_device *sdev = cmd->device;
>>   	struct request_queue *q = sdev->request_queue;
>>   
>> -	if (blk_update_request(req, error, bytes))
>> +	if (is_mpath_request(req)) {
>> +		if (scsi_mpath_end_request(req, error, bytes))
>> +			return true;
>> +	} else if (blk_update_request(req, error, bytes))
>>   		return true;
> 
> [Severity: Critical]
> If CONFIG_SCSI_MULTIPATH is disabled, the dummy stub for
> scsi_mpath_end_request() unconditionally returns false.
> 
> If a request has REQ_MPATH set (which aliases REQ_DRV and can be set by
> other subsystems like Device Mapper), won't the else if branch be completely
> skipped?

Nobody else should set it, but I can check this

> 
> Could this result in blk_update_request() never being called, leaking bios
> and hanging the submitting task waiting for completion?
> 
>> [ ... ]
>> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
>> index d79e1b7b90051..eae5864ce5826 100644
>> --- a/drivers/scsi/scsi_multipath.c
>> +++ b/drivers/scsi/scsi_multipath.c
>> [ ... ]
>> +bool scsi_mpath_end_request(struct request *req, blk_status_t error,
>> +				unsigned int nr_bytes)
>> +{
>> +	struct scsi_cmnd *scmd = blk_mq_rq_to_pdu(req);
>> +	struct scsi_device *sdev = scmd->device;
>> +	struct block_device *bi_bdev = NULL;
>> +
>> +	if (scmd->flags & SCMD_MPATH_IO_STATS) {
>> +		struct bio *clone = req->bio, *master = clone->bi_private;
>> +
>> +		bi_bdev = master->bi_bdev;
>> +	}
> 
> [Severity: Critical]
> If a request is requeued, SCMD_MPATH_IO_STATS and SCMD_MPATH_CNT_ACTIVE are
> preserved by SCMD_PRESERVED_FLAGS.

Yes, as sashiko suggested.

> If the request later fails an early state
> check in scsi_queue_rq() (e.g., the device goes offline), blk-mq ends it
> directly via blk_mq_end_request(), bypassing scsi_end_request().
> 
> When the tag is reused for a dataless request, scsi_mpath_start_request()
> returns early without clearing the flags. Will this cause
> scsi_mpath_end_request() to read the leaked SCMD_MPATH_IO_STATS flag and
> dereference req->bio which is NULL?
> 

ok, I can clear those flags