Re: [PATCH] ibmvscsi: implement vio driver shutdown call back to quiesce ibmvscsi

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] scsi: ibmvscsi: Use-After-Free in queuecommand due to premature resource free
- [High] scsi: ibmvscsi: deadlock during device removal due to blocked host
--

commit 83656c60ea18229cd02899fae2cde7d1ac88c182
Author: Tyrel Datwyler <tyreld@linux.ibm.com>

ibmvscsi: implement vio driver shutdown call back to quiesce ibmvscsi

This patch implements a driver shutdown callback to quiesce the ibmvscsi
driver and avoids tearing down the full host. It moves quiesce steps into
a new helper function, which is also used by the device remove path.

> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
> index 609bda730b3a8..77f4eac828788 100644
> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
> @@ -2341,26 +2341,39 @@ static int ibmvscsi_probe(struct vio_dev *vdev, const struct vio_device_id *id)
>  	return -1;
>  }
>  
> -static void ibmvscsi_remove(struct vio_dev *vdev)
> +static void ibmvscsi_quiesce(struct ibmvscsi_host_data *hostdata, int scsi_error_code)
>  {
> -	struct ibmvscsi_host_data *hostdata = dev_get_drvdata(&vdev->dev);
> -
> -	srp_remove_host(hostdata->host);
> -	scsi_remove_host(hostdata->host);
> -
> -	purge_requests(hostdata, DID_ERROR);
> -	release_event_pool(&hostdata->pool, hostdata);
> +	scsi_block_requests(hostdata->host);
> +	kthread_stop(hostdata->work_thread);
>  
>  	ibmvscsi_release_crq_queue(&hostdata->queue, hostdata,
>  					max_events);
>  
> -	kthread_stop(hostdata->work_thread);
> +	purge_requests(hostdata, scsi_error_code);
> +
> +	release_event_pool(&hostdata->pool, hostdata);

[Severity: Critical]
Will this lead to a use-after-free when handling concurrent SCSI commands?

If a concurrent scsi_queue_rq() thread passes the budget check before
host_self_blocked is set by scsi_block_requests(), it will block on
host_lock.

Once purge_requests() drops the host_lock, the preempted thread will
acquire it and enter ibmvscsi_queuecommand_lck(). Because the event pool
memory is freed by release_event_pool() right after purge_requests(),
get_event_struct() would directly access the freed pool->events array when
the preempted thread finally runs.

>  	unmap_persist_bufs(hostdata);
> +}
> +
> +static void ibmvscsi_shutdown(struct vio_dev *vdev)
> +{
> +	struct ibmvscsi_host_data *hostdata = dev_get_drvdata(&vdev->dev);
> +
> +	ibmvscsi_quiesce(hostdata, DID_REQUEUE);
> +}

[Severity: Critical]
Is it safe to omit scsi_remove_host() during shutdown?

During shutdown, the host is never removed here, leaving the SCSI midlayer
completely active. Could this cause crashes if I/O timeouts or commands
trigger after the driver resources have been quiesced and freed?

> +
> +static void ibmvscsi_remove(struct vio_dev *vdev)
> +{
> +	struct ibmvscsi_host_data *hostdata = dev_get_drvdata(&vdev->dev);
> +
> +	ibmvscsi_quiesce(hostdata, DID_ERROR);
>  
>  	spin_lock(&ibmvscsi_driver_lock);
>  	list_del(&hostdata->host_list);
>  	spin_unlock(&ibmvscsi_driver_lock);
>  
> +	srp_remove_host(hostdata->host);
> +	scsi_remove_host(hostdata->host);

[Severity: High]
Does this sequence cause a deadlock during device unbind or module unload?

Since ibmvscsi_quiesce() calls scsi_block_requests(), the host is put into a
blocked state before the child devices are removed.

When scsi_remove_host() synchronously unregisters child devices,
unregistering a SCSI disk calls sd_sync_cache() to flush the write cache.
This submits a bio and waits for completion via blk_execute_rq().

Because the host is blocked, the block layer budget check fails and returns
BLK_STS_RESOURCE, causing the block layer to continuously requeue the flush
request. Will the cache flush request wait indefinitely and deadlock the
rmmod or unbind process?

>  	scsi_host_put(hostdata->host);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916000626.1981569-1-tyreld@linux.ibm.com?part=1