Re: [PATCH v4 16/28] scsi-multipath: clone each bio

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] NULL pointer dereference due to missing mandatory template callbacks
- [High] Missing failover handling in clone completion
- [High] Bio allocation deadlock due to missing BIOSET_NEED_RESCUER
- [Medium] Memory leak due to missing ida_destroy()
--

--- Patch [16]: [PATCH v4 16/28] scsi-multipath: clone each bio ---
commit a74e2254f4409451e96518ce4d539e122559fac1
Author: John Garry <john.garry@linux.dev>

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 6228ff4be4ff7..9a4b35c597778 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c
> @@ -97,6 +97,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);
>  	mpath_head_uninit(mpath_head);
>  	kfree(scsi_mpath_head);

[Severity: Medium]
Will this leak memory if the embedded IDA isn't destroyed?

Even though empty IDAs free their bitmaps, the XArray API contract requires
calling ida_destroy() or xa_destroy() to ensure all internally allocated
memory is fully released. Can we add a call to
ida_destroy(&scsi_mpath_head->ida) before freeing the structure?

[ ... ]
> @@ -229,7 +230,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 skip all multipath failover logic on error?

When an underlying SCSI path fails, the error status is unconditionally
propagated to the master bio and completed. Should this be checking
blk_path_error() and requeueing the master bio to the requeue_list for a
retry on an alternate path instead?

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

[Severity: Critical]
Will this trigger a NULL pointer dereference?

The template omits the .is_disabled, .is_optimized, .available_path, and
.remove_head callbacks. The libmultipath core does not check if these are
NULL before calling them. For example, submitting any bio to the device
triggers __mpath_find_path() which calls:

lib/multipath.c:mpath_path_is_disabled() {
    return mpath_head->mpdt->is_disabled(mpath_device);
}

Should these mandatory callbacks be implemented in the template?

>  
>  static struct scsi_mpath_head *scsi_mpath_alloc_head(void)
> @@ -252,6 +280,9 @@ static struct scsi_mpath_head *scsi_mpath_alloc_head(void)
>  	scsi_mpath_head->index = ida_alloc(&scsi_multipath_dev_ida, GFP_KERNEL);
>  	if (scsi_mpath_head->index < 0)
>  		goto out_uninit_head;
> +	if (bioset_init(&scsi_mpath_head->bio_pool, BIO_POOL_SIZE,
> +			0, BIOSET_PERCPU_CACHE))

[Severity: High]
Could this cause a system deadlock?

The bioset is initialized without the BIOSET_NEED_RESCUER flag. Later,
scsi_mpath_clone_bio() allocates from this pool via bio_alloc_clone(). Since
this allocation occurs inside submit_bio_noacct(), an exhausted mempool will
block waiting for completions.

Without BIOSET_NEED_RESCUER, the block layer's deadlock avoidance cannot
intervene. Should BIOSET_NEED_RESCUER be added to the flags here?

> +		goto out_ida_free;
>  	kref_init(&scsi_mpath_head->ref);
>  
>  	device_initialize(&scsi_mpath_head->dev);

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