Re: [PATCH v9 19/29] scsi-multipath: failover handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] scsi-multipath: infinite bio retry loop in
  scsi_mpath_clone_end_io()
- [Medium] scsi: multipath: premature master_bio publish in
  scsi_mpath_clone_end_io()
- [Low] scsi-multipath: commit message describes scsi_cmnd request
  failover
--

commit 8db30403f561c839e4114d28ecdf086eb7458c31
Author: John Garry <john.garry@linux.dev>

scsi-multipath: failover handling

This commit adds failover handling to scsi_mpath_clone_end_io() by
requeueing the master bio onto mpath_head->requeue_list and scheduling
requeue work when a cloned bio fails with blk_path_error().

> Failover occurs when the scsi_cmnd has failed and it is discovered that the
> target scsi_device has transport down.
> 
> For a scsi command which suffers failover, requeue the master bio of each
> bio attached to its request.

[Severity: Low]
This isn't a bug, but do these first two paragraphs describe an earlier
request-based design?  The patch handles failover in
scsi_mpath_clone_end_io() based on blk_path_error(clone->bi_status) and
does not inspect scsi_cmnd, check whether the scsi_device transport is
down, or iterate over bios attached to a request.

> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> index 008bcafe73ad2..4c965650c2bd3 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c
> @@ -262,6 +262,20 @@ static void scsi_mpath_clone_end_io(struct bio *clone)
>  {
>  	struct bio *master_bio = clone->bi_private;
>  
> +	if (clone->bi_status && blk_path_error(clone->bi_status)) {

[Severity: High]
Can requeueing master_bio on every blk_path_error() without checking or
disabling the path cause an infinite bio retry loop?

blk_path_error() returns true for any status other than BLK_STS_NOTSUPP,
BLK_STS_NOSPC, BLK_STS_TARGET, BLK_STS_RESV_CONFLICT, BLK_STS_MEDIUM, and
BLK_STS_PROTECTION.  When a command fails with a status such as
BLK_STS_IOERR (for example, from ABORTED_COMMAND or DID_ERROR) while the
scsi_device remains in SDEV_RUNNING, scsi_mpath_is_disabled() stays false
and mpath_head->current_path[node] is not cleared.  mpath_requeue_work()
then calls submit_bio_noacct(master_bio) -> mpath_bdev_submit_bio() ->
mpath_find_path(), which repeatedly selects the same active path and
resubmits master_bio in an unbounded loop on kblockd.

> +		struct mpath_head *mpath_head =
> +				master_bio->bi_bdev->bd_disk->private_data;
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&mpath_head->requeue_lock, flags);
> +		bio_list_add(&mpath_head->requeue_list, master_bio);
> +		spin_unlock_irqrestore(&mpath_head->requeue_lock, flags);
> +		bio_put(clone);
> +
> +		mpath_schedule_requeue_work(mpath_head);

[Severity: Medium]
Can releasing mpath_head->requeue_lock before bio_put(clone) and
mpath_schedule_requeue_work(mpath_head) cause a use-after-free on
scsi_mpath_head->bio_pool and mpath_head?

Because mpath_bdev_submit_bio() calls submit_bio_noacct(clone) while
current->bio_list is active, mpath_bdev_submit_bio() drops
mpath_head->srcu before __submit_bio_noacct() submits clone to the
underlying queue.  If clone fails during queue entry (such as on a dying
queue) or completes asynchronously after request teardown,
scsi_mpath_clone_end_io() runs without mpath_head->srcu or the underlying
q_usage_counter held.  Once requeue_lock is dropped, an already-running
mpath_requeue_work() on another CPU can dequeue and complete master_bio
 while the last device reference is being dropped:

CPU0                                  CPU1
scsi_mpath_clone_end_io()
  spin_lock_irqsave(&requeue_lock)
  bio_list_add(&requeue_list,
               master_bio)
  spin_unlock_irqrestore(&requeue_lock)
                                      mpath_requeue_work()
                                        bio_list_get(&requeue_list)
                                        submit_bio_noacct(master_bio)
                                          ...
                                            bio_io_error(master_bio)
                                      ...
                                      scsi_mpath_head_release()
                                        bioset_exit(&bio_pool)
                                        kfree(scsi_mpath_head)
  bio_put(clone)  <-- use-after-free
  mpath_schedule_requeue_work(
    mpath_head)   <-- use-after-free

Calling bio_put(clone) before acquiring mpath_head->requeue_lock and
calling mpath_schedule_requeue_work(mpath_head) while holding
mpath_head->requeue_lock avoids this race.

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