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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] List corruption and use-after-free occur due to `bio_list_add_clone()` failing to nullify `master_bio->bi_next`.
- [High] A use-after-free crash occurs because `cancel_work_sync()` is missing during `mpath_head` teardown.
--

--- Patch [18]: [PATCH v6 18/27] scsi-multipath: failover handling ---
commit d3d46b0330ddb0dc22577a7546f9765b9e1a4651
Author: John Garry <[email protected]>

scsi-multipath: failover handling
    
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 12750621a7fc0..6ced89576368c 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c
> @@ -256,10 +256,49 @@ void scsi_mpath_dev_clear_path(struct scsi_mpath_device *scsi_mpath_dev)
[ ... ]
> +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;
> +	bio_put(clone);
> +}

[Severity: High]
Will this leave the newly appended list element improperly terminated?

This custom insertion into requeue_list appears to omit the standard
master_bio->bi_next = NULL assignment for the new list tail.

If master_bio->bi_next happens to contain a stale pointer when it is added
here, could iterating the list later result in memory corruption or memory
being accessed after it has been freed?

[ ... ]
> +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)) {
[ ... ]
> +		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]
Is it possible for the scheduled requeue work item to run after the
mpath_head structure has been torn down?

If a device hot-unplug overlaps with I/O path errors, requeue_work is
scheduled here.

When teardown proceeds to remove the device and drops the last reference,
it appears scsi_mpath_head_release() frees the scsi_mpath_head structure
without calling cancel_work_sync() first.

Could this sequence lead to a delayed kworker executing on deallocated
memory?

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