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

[sashiko-bot@kernel.org]

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 <john.garry@linux.dev>
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?

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

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