Re: [PATCH v4 19/28] scsi-multipath: failover handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Missing bi_next clearing in bio_list_add_clone() leads to corrupted bio requeue list.
- [High] Releasing the SCSI device via scsi_device_put() causes a module reference count underflow due to mismatched API usage.
- [High] Uninitialized mpath_device->mpath_head leads to a guaranteed NULL pointer dereference during failover.
- [High] The scsi_mpath_clone_end_io handler dereferences mpath_head after it has been freed during path device removal.
- [High] Missing cancel_work_sync() allows requeue_work to execute after scsi_mpath_head is freed.
--

--- Patch [19]: [PATCH v4 19/28] scsi-multipath: failover handling ---
commit e8daaaa500b47bda767344af0f467a56fa0a6095
Author: John Garry <john.garry@linux.dev>

scsi-multipath: failover handling

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.

> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> index b896d28a7dd9a..a482b6ede21aa 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c
> @@ -248,11 +248,53 @@ void scsi_mpath_dev_clear_path(struct scsi_mpath_device *scsi_mpath_dev)
>                 mpath_synchronize(mpath_head);
>  }
>  
> +static inline void bio_list_add_clone(struct bio_list *bl,
> +				struct bio *clone)
> +{
> +	struct bio *master_bio = clone->bi_private;
> +
> +	if (bl->tail)
> +		bl->tail->bi_next = master_bio;
> +	else
> +		bl->head = master_bio;
> +	bl->tail = master_bio;

[Severity: High]
Since master_bio is appended to the tail of the bio_list, shouldn't its
bi_next pointer be explicitly cleared to null to properly terminate the
singly-linked list? 

When mpath_requeue_work() later iterates through the list, couldn't this
cause it to follow a stale bi_next pointer past the end of the list?

> +	bio_put(clone);
> +}
> +
>  static void scsi_mpath_clone_end_io(struct bio *clone)
>  {
>  	struct bio *master_bio = clone->bi_private;
>  
>  	master_bio->bi_status = clone->bi_status;
> +
> +	if (clone->bi_status && blk_path_error(clone->bi_status)) {
> +		struct block_device *bi_bdev = clone->bi_bdev;
> +		struct request_queue *q = bi_bdev->bd_queue;
> +		struct scsi_device *sdev = scsi_device_from_queue(q);
> +		struct scsi_mpath_device *scsi_mpath_dev;
> +		struct mpath_device *mpath_device;
> +		struct mpath_head *mpath_head;
> +		unsigned long flags;
> +
> +		if (!sdev) {
> +			master_bio->bi_status = clone->bi_status;
> +			goto end_bio;
> +		}
> +
> +		scsi_mpath_dev = sdev->scsi_mpath_dev;
> +		mpath_device = &scsi_mpath_dev->mpath_device;
> +		mpath_head = mpath_device->mpath_head;

[Severity: High]
Is mpath_device->mpath_head guaranteed to be initialized here? 

If scsi_mpath_dev_alloc() initializes scsi_mpath_dev with kzalloc but never
assigns this pointer, wouldn't this cause a null pointer dereference when
accessing requeue_lock below?

[Severity: High]
Could an in-flight I/O completion racing with path removal extract a freed
mpath_head here? 

If scsi_mpath_remove_device() drops the reference to scsi_mpath_head and
frees it, but sdev->scsi_mpath_dev remains valid, it seems this could lead
to a use-after-free when accessing its requeue_lock.

> +
> +		spin_lock_irqsave(&mpath_head->requeue_lock, flags);
> +		bio_list_add_clone(&mpath_head->requeue_list, clone);
> +		spin_unlock_irqrestore(&mpath_head->requeue_lock, flags);
> +
> +		mpath_schedule_requeue_work(mpath_head);

[Severity: High]
When we schedule this requeue work, is there a corresponding cancel_work_sync()
during cleanup?

Looking at scsi_mpath_head_release(), it frees scsi_mpath_head without
cancelling or flushing mpath_head->requeue_work. Could this allow a
use-after-free if the background worker runs after the head is freed?

> +		scsi_device_put(sdev);

[Severity: High]
Will this cause a module reference count underflow?

scsi_device_from_queue() only acquires a device reference via get_device(),
but scsi_device_put() drops both the device and module reference. Should
this use put_device(&sdev->sdev_gendev) instead?

> +		return;
> +	}
> +
> +end_bio:
>  	bio_put(clone);
>  	bio_endio(master_bio);
>  }

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