Re: [PATCH v9 21/29] scsi-multipath: add scsi_mpath_{start,end}_request()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] scsi: multipath: premature bio completion in
  scsi_mpath_end_request()
- [Medium] scsi-multipath: mismatched bdev I/O accounting in
  scsi_mpath_end_request()
--

commit 29255340e39c30a34f5cb1da674871fdd2c627ea
Author: John Garry <john.garry@linux.dev>

scsi-multipath: add scsi_mpath_{start,end}_request()

This commit adds scsi_mpath_start_request() and scsi_mpath_end_request()
to track per-host active multipath requests for the queue-depth I/O policy
and perform block device I/O accounting on multipath requests.

> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> index 2f47c39d82936..93a4545f1dbf7 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c
> @@ -545,6 +545,75 @@ void scsi_mpath_put_head(struct scsi_mpath_head *scsi_mpath_head)
>  }
>  EXPORT_SYMBOL_GPL(scsi_mpath_put_head);
>  
> +void scsi_mpath_start_request(struct request *req)
> +{
> +	struct scsi_cmnd *scmd = blk_mq_rq_to_pdu(req);
> +	struct scsi_device *sdev = scmd->device;
> +	struct scsi_mpath_device *scsi_mpath_dev = sdev->scsi_mpath_dev;
> +	struct scsi_mpath_head *scsi_mpath_head =
> +				scsi_mpath_dev->scsi_mpath_head;
> +	struct bio *clone = req->bio, *master;
> +	struct mpath_head *mpath_head = &scsi_mpath_head->mpath_head;
> +	struct gendisk *disk = mpath_head->disk;
> +
> +	if (mpath_qd_iopolicy(&scsi_mpath_head->iopolicy) &&
> +	    !(scmd->flags & SCMD_MPATH_CNT_ACTIVE)) {
> +		struct Scsi_Host *shost = sdev->host;
> +
> +		atomic_inc(&shost->mpath_nr_active);
> +		scmd->flags |= SCMD_MPATH_CNT_ACTIVE;
> +	}
> +
> +	if (blk_rq_is_passthrough(req) || !blk_queue_io_stat(disk->queue))
> +		return;
> +
> +	if (!clone) {
> +		scmd->flags &= ~SCMD_MPATH_IO_STATS;
> +		return;
> +	}
> +
> +	if (scmd->flags & SCMD_MPATH_IO_STATS)
> +		return;
> +
> +	master = clone->bi_private;
> +
> +	scmd->flags |= SCMD_MPATH_IO_STATS;
> +	scmd->start_time = bdev_start_io_acct(master->bi_bdev, req_op(req),
> +				jiffies);
> +	scmd->start_bytes = blk_rq_bytes(req);
> +}
> +
> +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: Medium]
Can scsi_mpath_start_request() and scsi_mpath_end_request() account I/O
against different block_device structures when a merged request completes
partially?

scsi_mpath_clone_bio() directs all clone bios to the underlying path
disk's part0 while storing the original bio in clone->bi_private.
Contiguous clone bios from adjacent partitions on mpath_head->disk can
therefore be merged into a single request on sdev->request_queue.

When that request starts, scsi_mpath_start_request() calls
bdev_start_io_acct() on the first bio's master->bi_bdev and sets
SCMD_MPATH_IO_STATS.  If a partial completion advances req->bio past the
first bio (returning true from blk_update_request()), the final call to
scsi_mpath_end_request() re-derives bi_bdev from the remaining bio's
master->bi_bdev and calls bdev_end_io_acct() on the second partition,
leaking in_flight on the first partition and underflowing it on the
second.

Accounting against mpath_head->disk->part0 in both functions (or saving
the accounted block_device) would keep the start and end calls paired.

> +
> +	if (blk_update_request(req, error, nr_bytes))
> +		return true;
> +
> +	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 false;
> +	bdev_end_io_acct(bi_bdev, req_op(req),
> +			 scmd->start_bytes >> SECTOR_SHIFT,
> +			 scmd->start_time);
> +
> +	return false;
> +}

[Severity: Medium]
Does calling blk_update_request() before updating shost->mpath_nr_active
and calling bdev_end_io_acct() complete master_bio too early?

blk_update_request() runs bio_endio(clone) -> scsi_mpath_clone_end_io(),
which either schedules mpath_head->requeue_work on a path error or calls
bio_endio(master_bio).  Because mpath_head->disk is bio-based,
__submit_bio() drops its queue reference as soon as
mpath_bdev_submit_bio() returns, so nothing pins bi_bdev once master_bio
completes:

CPU0                                  CPU1
scsi_mpath_end_request()
  blk_update_request()
    ...
      scsi_mpath_clone_end_io()
        bio_endio(master_bio)
                                      blkdev_put()
                                      bdev_del_partition()
                                        ...
                                        bdev_free_inode()
                                          free_percpu(bdev->bd_stats)
  bdev_end_io_acct(bi_bdev, ...)
    <-- use-after-free on bi_bdev

In addition, when scsi_mpath_clone_end_io() triggers failover or a
caller immediately submits dependent I/O under MPATH_IOPOLICY_QD,
mpath_queue_depth_path() reads shost->mpath_nr_active before this request
has decremented it.

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