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

[John Garry <john.garry@linux.dev>]

On 9/7/26 13:02, sashiko-bot@kernel.org wrote:
> 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.

As mentioned, The NULLify of master_bio->bi_next is unneeded. But I 
suppose we can use bio_list_add() - I can do it later

> 
>> +
>>   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. 

We don't support them.

> 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);
>>
>