Re: [PATCH v8 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 2 potential issue(s) to consider:
- [High] Failure to cancel pending `notoper_work` during device release causes a workqueue leak and use-after-free on the `private` structure.
- [High] Missing `io_mutex` locking in `vfio_ccw_notoper_todo` leads to a data race with concurrent channel program operations.
--

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

s390/vfio_ccw: move cp cleanup out of not operational

The fsm_notoper() routine is called when the device has been lost, and is
no longer operational. Push the cleanup of the private->cp resources to a
workqueue, where it can be done safely under a common locking mechanism.

> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -125,6 +125,15 @@ void vfio_ccw_crw_todo(struct work_struct *work)
>  		eventfd_signal(private->crw_trigger);
>  }
>  
> +void vfio_ccw_notoper_todo(struct work_struct *work)
> +{
> +	struct vfio_ccw_private *private;
> +
> +	private = container_of(work, struct vfio_ccw_private, notoper_work);
> +
> +	cp_free(&private->cp);
> +}
> +

[Severity: High]
The commit message mentions performing the cleanup safely under a common
locking mechanism. Could this lockless call to cp_free() lead to a regression
with concurrent channel program operations?

If userspace initiates an I/O request, vfio_ccw_mdev_write_io_region()
holds io_mutex and processes the request via fsm_io_request() and
cp_init() / cp_prefetch().

Concurrently, if a hardware event triggers fsm_notoper() and queues
notoper_work, this worker executes vfio_ccw_notoper_todo() and locklessly
calls cp_free(&private->cp).

Should this function acquire io_mutex to prevent a data race with the
allocation and initialization of ccwchain structures in the I/O path?

> 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

[ ... ]

> @@ -133,7 +134,9 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
>  
>  	/*
>  	 * Ensure these work items are fully drained, so none can
> -	 * fire after being released.
> +	 * fire after being released. The notoper_work struct is
> +	 * only meaningful if the device had been opened, which
> +	 * means it would have been cleaned in an earlier close.
>  	 */
>  	cancel_work_sync(&private->io_work);
>  	cancel_work_sync(&private->crw_work);

[Severity: High]
Is the assumption that notoper_work is only meaningful if the device had
been opened always correct?

If a machine check or path loss occurs before the mediated device is opened,
vfio_ccw_sch_event() transitions the state from STANDBY to NOT_OPER via
fsm_notoper(), queuing notoper_work unconditionally.

Since the device was never opened, vfio_ccw_mdev_close_device() (which
flushes the work) is never called.

When the device is subsequently removed, this release function omits canceling
notoper_work. This could allow the workqueue to execute vfio_ccw_notoper_todo()
after the VFIO core frees the private structure.

Should cancel_work_sync(&private->notoper_work) be added here to prevent a
potential use-after-free on the private structure?

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