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 <[email protected]> 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/[email protected]?part=19
