Re: [PATCH v9 22/29] scsi-multipath: update request for terminal request queuing error

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] scsi-multipath: NULL dereference on bio-less request in
  scsi_mpath_end_request_no_update()
--

commit 6015c0421ae4c77548304e8f223ec4e93492aa00
Author: John Garry <john.garry@linux.dev>

scsi-multipath: update request for terminal request queuing error

This commit adds scsi_mpath_end_request_no_update() and calls it from
scsi_queue_rq() when a multipath request fails with a terminal queueing
error, updating active request counts and I/O statistics.

> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> index 93a4545f1dbf7..bec6dd94f3be8 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c
> @@ -614,6 +614,41 @@ bool scsi_mpath_end_request(struct request *req, blk_status_t error,
>  	return false;
>  }
>  
> +void scsi_mpath_end_request_no_update(struct request *req, blk_status_t error)
> +{
> +	struct scsi_cmnd *scmd = blk_mq_rq_to_pdu(req);
> +	struct scsi_device *sdev = scmd->device;
> +	struct bio *clone = req->bio, *master = clone->bi_private;
> +	struct block_device *bi_bdev = master->bi_bdev;
> +	unsigned int nr_bytes;
> +
> +	/* Don't generate error logs if we are going to try another path */
> +	if (blk_path_error(error))
> +		req->rq_flags |= RQF_QUIET;
> +
> +	if (scmd->flags & SCMD_MPATH_CNT_ACTIVE) {
> +		struct Scsi_Host *shost = sdev->host;
> +
> +		atomic_dec_if_positive(&shost->mpath_nr_active);
> +	}
> +
> +	if (!(scmd->flags & SCMD_MPATH_IO_STATS))
> +		return;

[Severity: High]
Can clone->bi_private cause a NULL pointer dereference when req->bio is
NULL?

blk_kick_flush() initializes fq->flush_rq with flush_rq->bio set to NULL
and copies REQ_DRV (which REQ_MPATH aliases) into flush_rq->cmd_flags:

block/blk-flush.c:blk_kick_flush() {
    ...
	flush_rq->cmd_flags = REQ_OP_FLUSH | REQ_PREFLUSH;
	flush_rq->cmd_flags |= (flags & REQ_DRV) | (flags & REQ_FAILFAST_MASK);
    ...
}

Unlike scsi_mpath_start_request() and scsi_mpath_end_request(), which
only dereference req->bio when SCMD_MPATH_IO_STATS is set,
scsi_mpath_end_request_no_update() dereferences req->bio unconditionally
at function entry before checking SCMD_MPATH_IO_STATS.  If a flush
request hits a terminal queueing error in scsi_queue_rq() (such as when a
path is transport offline), this crashes.  Deferring the req->bio and
clone->bi_private dereferences until after the SCMD_MPATH_IO_STATS check
would avoid this.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108204733.557995-1-john.garry@linux.dev?part=22