Re: [PATCH v6 08/10] s390/vfio_ccw: move cp cleanup out of not operational

[sashiko-bot@kernel.org]

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

New issues:
- [High] Memory leak / API violation due to pinned pages remaining indefinitely when `dma_unmap` is called on a not-operational device.

Pre-existing issues:
- [High] Use-After-Free or Double Free of channel program resources due to concurrent execution of `fsm_close()` and asynchronous workqueues.
- [High] Data Race leading to Use-After-Free between `vfio_ccw_dma_unmap()` and `cp_free()`.
- [High] Data Race causing List Corruption in the `private->crw` list.
--

--- Patch [8]: [PATCH v6 08/10] s390/vfio_ccw: move cp cleanup out of not operational ---
commit 95b263299e23889427ae40eed564ae801fb6f8d2
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: move cp cleanup out of not operational

This patch defers the cleanup of the private channel program resources
from the fsm_notoper routine to the closure of the MDEV device. This aims to
prevent lock contention issues when the device asynchronously becomes not
operational.

> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> index 4d7988ea47ef0..248adf1e77073 100644
> --- a/drivers/s390/cio/vfio_ccw_fsm.c
> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> @@ -169,9 +169,6 @@ static void fsm_notoper(struct vfio_ccw_private *private,
>  	 */
>  	css_sched_sch_todo(sch, SCH_TODO_UNREG);
>  	private->state = VFIO_CCW_STATE_NOT_OPER;
> -
> -	/* This is usually handled during CLOSE event */
> -	cp_free(&private->cp);

[Severity: High]
Does removing this cleanup leave pages pinned indefinitely if the device enters
the NOT_OPER state?

If a guest unmaps DMA memory after the device enters VFIO_CCW_STATE_NOT_OPER,
vfio_ccw_dma_unmap() calls vfio_ccw_mdev_reset().

However, in the NOT_OPER state, the FSM maps VFIO_CCW_EVENT_CLOSE to fsm_nop.
This causes the reset to return -EINVAL without freeing the pinned pages.

Could this leak the memory and violate the VFIO API contract?

>  }
>  
>  /*

[ ... ]

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index bd488e40e1531..a717174675bd6 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -210,6 +210,15 @@ static void vfio_ccw_mdev_close_device(struct vfio_device *vdev)
>  	cancel_work_sync(&private->io_work);

[Severity: High]
This is a pre-existing issue, but can vfio_ccw_mdev_close_device() trigger a
use-after-free by dispatching the close event before canceling the I/O
workqueues?

In vfio_ccw_mdev_close_device(), vfio_ccw_fsm_event() emits the close event,
which invokes fsm_close().

fsm_close() unconditionally calls cp_free() before cancel_work_sync()
completes.

Concurrently, the asynchronous workqueue vfio_ccw_sch_io_todo() can execute
and also conditionally call cp_free() locklessly.

Does this cause a double-free or use-after-free of the channel program
resources?

>  	cancel_work_sync(&private->crw_work);

[Severity: High]
This isn't a bug introduced by this patch, but does the CRW list suffer from
data races that could corrupt the list?

Looking at the CRW region handling, vfio_ccw_queue_crw() adds items to
private->crw using list_add_tail().

Conversely, vfio_ccw_crw_region_read() removes items using list_del().

Since both operate locklessly without a spinlock or mutex, could concurrent
execution corrupt the kernel linked list pointers?

> +	/*
> +	 * Ensure that a not-operational device has indeed
> +	 * freed any resources. Normally the interrupt handler
> +	 * or FSM close should do this, but there exists the
> +	 * possibility that a device removed from the config
> +	 * would have left some of this about.
> +	 */
> +	cp_free(&private->cp);

[Severity: High]
This is a pre-existing issue, but does calling cp_free() concurrently with
DMA unmap operations lead to a use-after-free?

In cp_free(), the channel program list is traversed and its elements are freed
locklessly:

drivers/s390/cio/vfio_ccw_cp.c:cp_free() {
    ...
    list_for_each_entry_safe(chain, temp, &cp->ccwchain_list, next) {
        ...
        ccwchain_free(chain);
    }
}

Meanwhile, vfio_ccw_dma_unmap() invokes cp_iova_pinned(), which traverses the
exact same ccwchain_list without holding any locks.

If cp_free() unlinks and frees the chains while cp_iova_pinned() is
traversing them, could this result in a use-after-free?

> +
>  	vfio_ccw_unregister_dev_regions(private);
>  }

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