Re: [PATCH v8 18/27] scsi-multipath: failover handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing cancellation of `requeue_work` during `scsi_mpath_head` destruction leads to Use-After-Free.
- [Low] The custom `bio_list_add_master()` function is an unnecessary and unsafe reinvention of `bio_list_add()`.
--

commit 9189539f81df33ce64a7c19420baf37ebf587b37
Author: John Garry <john.garry@linux.dev>

scsi-multipath: failover handling

This patch implements failover handling for SCSI commands when the target
device's transport is down. It requeues the master bio of each attached
bio when a failover occurs, using scsi_mpath_clone_end_io() to handle
the failover process.

> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> index 456351fee4b98..12200664ed676 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c
> @@ -256,10 +256,34 @@ void scsi_mpath_dev_clear_path(struct scsi_mpath_device *scsi_mpath_dev)
>                 mpath_synchronize(mpath_head);
>  }
>  
> +static inline void bio_list_add_master(struct bio_list *bl,
> +				struct bio *master_bio)
> +{
> +	if (bl->tail)
> +		bl->tail->bi_next = master_bio;
> +	else
> +		bl->head = master_bio;
> +	bl->tail = master_bio;
> +}

[Severity: Low]
Is it necessary to use this custom list insertion function instead of the
standard bio_list_add()?

This custom bio_list_add_master() function mimics bio_list_add() but omits
the master_bio->bi_next = NULL assignment. While master_bio->bi_next is
structurally guaranteed to be NULL in the block layer at this point,
reinventing bio_list_add() without its safety guarantees seems to add
unnecessary complexity and a theoretical risk of singly-linked list
corruption.

> +
>  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)) {
> +		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_master(&mpath_head->requeue_list, master_bio);
> +		spin_unlock_irqrestore(&mpath_head->requeue_lock, flags);
> +		bio_put(clone);
> +
> +		mpath_schedule_requeue_work(mpath_head);

[Severity: High]
Can scheduling this requeue work lead to a use-after-free during device
destruction?

While the I/O failover path here is protected for disk devices because
del_gendisk() blocks until the bio completes, non-disk SCSI devices
(like tape drives) do not create an mpath disk. If an ALUA state change
event triggers scsi_mpath_revalidate_paths(), requeue_work can be scheduled.

If the device is hot-removed, scsi_mpath_head_release() frees the
scsi_mpath_head structure without calling
cancel_work_sync(&mpath_head->requeue_work):

drivers/scsi/scsi_multipath.c:scsi_mpath_head_release() {
    ...
    mpath_head_uninit(mpath_head);
    kfree(scsi_mpath_head);
}

If asynchronous mpath_requeue_work() executes after the structure is freed,
it could access the freed mpath_head structure and cause a kernel panic.

> +		return;
> +	}
> +
>  	master_bio->bi_status = clone->bi_status;
>  	bio_put(clone);
>  	bio_endio(master_bio);
>

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