Re: [PATCH v6 09/10] s390/vfio_ccw: implement a channel program mutex

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] A 3-thread AB-BC-CA deadlock is introduced involving `cp_mutex`, `iommu->lock`, and `iommu->device_list_lock`.
--

--- Patch [9]: [PATCH v6 09/10] s390/vfio_ccw: implement a channel program mutex ---
commit 24b60b7244a72b137b8847a953a24223d19d152e
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: implement a channel program mutex

This patch introduces a mutex to protect the channel program struct in the
vfio_ccw driver. The lock is held across operations that require private->cp
to ensure consistent behavior and replaces some incorrect subchannel lock uses.

> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> --- a/drivers/s390/cio/vfio_ccw_fsm.c
> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> @@ -249,6 +246,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
>  	private->state = VFIO_CCW_STATE_CP_PROCESSING;
>  	memcpy(scsw, io_region->scsw_area, sizeof(*scsw));
>  
> +	mutex_lock(&private->cp_mutex);
> +
>  	if (scsw->cmd.fctl & SCSW_FCTL_START_FUNC) {
>  		orb = (union orb *)io_region->orb_area;

[ ... ]

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -38,8 +38,13 @@ static void vfio_ccw_dma_unmap(struct vfio_device *vdev, u64 iova, u64 length)
>  		container_of(vdev, struct vfio_ccw_private, vdev);
>  
>  	/* Drivers MUST unpin pages in response to an invalidation. */
> -	if (!cp_iova_pinned(&private->cp, iova, length))
> +	mutex_lock(&private->cp_mutex);

[Severity: High]
Could this introduce a 3-way deadlock with the VFIO IOMMU subsystem?

When an I/O request is submitted, fsm_io_request() takes cp_mutex and
eventually calls cp_prefetch(), which attempts to acquire iommu->lock via
vfio_pin_pages().

At the same time, if a VFIO DMA unmap occurs, the VFIO core calls this
vfio_ccw_dma_unmap() callback while already holding device_list_lock.
This function then attempts to acquire cp_mutex.

Meanwhile, a concurrent VFIO DMA unmap could acquire iommu->lock in the
VFIO core (which drops it temporarily) and attempt to acquire
device_list_lock.

This seems to create a circular dependency:

Thread 1 (I/O submission):
fsm_io_request()
  mutex_lock(&private->cp_mutex);
  cp_prefetch()
    ...
      mutex_lock(&iommu->lock);

Thread 2 (VFIO DMA unmap):
vfio_notify_dma_unmap()
  mutex_lock(&iommu->device_list_lock);
  vfio_ccw_dma_unmap()
    mutex_lock(&private->cp_mutex);

Thread 3 (Concurrent VFIO DMA unmap):
vfio_notify_dma_unmap()
  mutex_lock(&iommu->lock);
  mutex_lock(&iommu->device_list_lock);

Is it possible for this sequence to freeze the CCW device operations and
the VFIO IOMMU subsystem?

> +	if (!cp_iova_pinned(&private->cp, iova, length)) {
> +		mutex_unlock(&private->cp_mutex);
>  		return;
> +	}
> +
> +	mutex_unlock(&private->cp_mutex);
>  
>  	vfio_ccw_mdev_reset(private);
>  }

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