Re: [PATCH v2 6/7] s390/vfio_ccw: implement a channel program lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The newly introduced `cp_lock` spinlock is held while executing operations that acquire sleeping locks, leading to a guaranteed 'scheduling while atomic' kernel BUG.

Pre-existing issues:
- [High] Incomplete locking allows a data race between channel program initialization and `vfio_ccw_dma_unmap()`, leading to a NULL pointer dereference.
- [High] The background work items `crw_work` and `io_work` are not explicitly cancelled before the driver frees the device's private memory.
--

--- Patch [6]: [PATCH v2 6/7] s390/vfio_ccw: implement a channel program lock ---
commit fcac9cc035321189b03cc15ccc7af0d3409a1018
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: implement a channel program lock

The channel_program struct is manipulated without a serialization
mechanism to ensure consistent behavior. This patch introduces a spin
lock to guard the entire structure, which can be acquired with or
without the mutex guarding the I/O regions.

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -810,6 +810,17 @@ void cp_free(struct channel_program *cp)
>  	}
>  }
>  
> +void cp_free(struct channel_program *cp)
> +{
> +	struct vfio_ccw_private *private =
> +		container_of(cp, struct vfio_ccw_private, cp);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&private->cp_lock, flags);
> +	cp_free_locked(cp);

[Severity: High]
Does holding the cp_lock spinlock here lead to a scheduling while atomic bug?

When cp_free_locked() cascades into the unpinning logic:

cp_free_locked()
  page_array_unpin_free()
    vfio_unpin_pages()
      vfio_iommu_type1_unpin_pages()
        mutex_lock(&iommu->lock);

The IOMMU backend unconditionally acquires a sleeping lock while the
thread is in atomic context from the spin_lock_irqsave(). Can this crash
the kernel when I/O request cleanup executes?

> +	spin_unlock_irqrestore(&private->cp_lock, flags);
> +}
> +

[ ... ]

> 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
> @@ -36,10 +36,15 @@ static void vfio_ccw_dma_unmap(struct vfio_device *vdev, u64 iova, u64 length)
>  {
>  	struct vfio_ccw_private *private =
>  		container_of(vdev, struct vfio_ccw_private, vdev);
> +	unsigned long flags;
>  
>  	/* Drivers MUST unpin pages in response to an invalidation. */
> -	if (!cp_iova_pinned(&private->cp, iova, length))
> +	spin_lock_irqsave(&private->cp_lock, flags);
> +	if (!cp_iova_pinned(&private->cp, iova, length)) {

[Severity: High]
This is a pre-existing issue, but does adding the lock here expose a NULL
pointer dereference due to incomplete locking elsewhere?

In fsm_io_request(), cp_init() and cp_prefetch() are called without
holding the new cp_lock. cp_init() sets cp->initialized = true. Then
cp_prefetch() calls page_array_alloc(), which does:

page_array_alloc()
  pa->pa_nr = len;
  pa->pa_iova = kzalloc_objs(*pa->pa_iova, len);

If a concurrent unmap enters vfio_ccw_dma_unmap(), acquires cp_lock, and
iterates the channel program because initialized is true, can it
dereference the still-NULL pa->pa_iova array since the initializing
thread hasn't allocated it yet?

> +		spin_unlock_irqrestore(&private->cp_lock, flags);
>  		return;
> +	}
> +	spin_unlock_irqrestore(&private->cp_lock, flags);
>  
>  	vfio_ccw_mdev_reset(private);
>  }

[ ... ]

> @@ -54,6 +59,7 @@ static int vfio_ccw_mdev_init_dev(struct vfio_device *vdev)
>  	INIT_LIST_HEAD(&private->crw);
>  	INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
>  	INIT_WORK(&private->crw_work, vfio_ccw_crw_todo);
> +	spin_lock_init(&private->cp_lock);

[Severity: High]
This isn't a bug introduced by this patch, but do these work items leak
and cause a use-after-free on device teardown?

According to the subsystem guidelines, once queue_work() is called,
flush_work() and other workqueue shutdown methods prevent us from leaking
the work struct on shutdown.

If an asynchronous subchannel event fires and queues crw_work, and the
user concurrently removes the mdev device, vfio_ccw_mdev_release_dev()
frees the private structure:

vfio_ccw_mdev_release_dev()
  kfree(private->cp.guest_cp);
  mutex_destroy(&private->io_mutex);

Will the workqueue scheduler eventually execute the pending crw_work and
access the already-freed private structure, since the removal function
drops references without explicitly cancelling background work?

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