Re: [PATCH v6 15/27] scsi-multipath: clone each bio

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The `smpdt` structure fails to implement mandatory `mpath_head_template` callbacks, leading to a NULL pointer dereference.
- [High] The clone completion callback entirely lacks the failover resubmission logic claimed in the commit message, breaking failover handling.
- [High] The `bio_pool` mempool is destroyed before waiting for active users to drain, leading to a use-after-free when in-flight cloned bios complete.
--

--- Patch [15]: [PATCH v6 15/27] scsi-multipath: clone each bio ---
commit ea53159f3e5b8959582afd4b035267bb7a201a6d
Author: John Garry <[email protected]>

scsi-multipath: clone each bio

For failover handling, we will take the approach to resubmit each
bio.

However, unlike NVMe, for SCSI there is no guarantee that any bio submitted
is either all or none completed.

As such, for SCSI, for failover handling we will take the approach to
just re-submit the original bio. For this, clone and submit each bio.

> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> index 593f94aac70e9..0d439cac6447a 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c
> @@ -99,6 +99,7 @@ static void scsi_mpath_head_release(struct device *dev)
>  		container_of(dev, struct scsi_mpath_head, dev);
>  	struct mpath_head *mpath_head = &scsi_mpath_head->mpath_head;
>  
> +	bioset_exit(&scsi_mpath_head->bio_pool);
>  	ida_free(&scsi_multipath_dev_ida, scsi_mpath_head->index);
>  	ida_destroy(&scsi_mpath_head->ida);
>  	mpath_head_uninit(mpath_head);

[Severity: High]
Does this code destroy the mempool before all users have drained?

When the last SCSI path is removed, scsi_mpath_head_release() calls
bioset_exit() immediately. It then calls mpath_head_uninit(), which
synchronously waits for all users to release their references.

If there are in-flight cloned bios completing, or if the block device is
still open and receives new I/O while waiting, they could access the
already-freed bio_pool via bio_put() or bio_alloc_clone().

Can this result in a use-after-free on the destroyed mempool?

[ ... ]

> @@ -237,7 +238,34 @@ static int scsi_multipath_sdev_init(struct scsi_device *sdev)
>  	return 0;
>  }
>  
> +static void scsi_mpath_clone_end_io(struct bio *clone)
> +{
> +	struct bio *master_bio = clone->bi_private;
> +
> +	master_bio->bi_status = clone->bi_status;
> +	bio_put(clone);
> +	bio_endio(master_bio);
> +}

[Severity: High]
Does this callback omit the failover resubmission logic mentioned in the
commit message?

When an underlying SCSI path fails an I/O, this function unconditionally
copies the error to master_bio and calls bio_endio().

Since the commit message states the approach is to re-submit the original
bio for failover handling, shouldn't there be code here to intercept the
error and requeue it, rather than immediately returning the error to the
issuer?

[ ... ]

>  static struct mpath_head_template smpdt = {
> +	.clone_bio = scsi_mpath_clone_bio,
>  };

[Severity: High]
Is this struct missing mandatory mpath_head_template callbacks?

Any I/O submission to the multipath block device invokes mpath_find_path(),
which unconditionally calls mpath_head->mpdt->is_disabled(mpath_device).

Because smpdt only initializes .clone_bio, functions like is_disabled,
is_optimized, and available_path will be NULL.

Will this cause an immediate kernel panic due to a NULL pointer dereference
when an I/O is issued?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=15