Re: [PATCH v5 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:

New issues:
- [High] Canceling `notoper_work` during device release can silently skip the cleanup of channel program resources, causing a memory leak of pinned pages and ccwchains.

Pre-existing issues:
- [High] Asynchronous execution of `cp_free` in `vfio_ccw_notoper_todo` without locks races with other accesses to `private->cp`, leading to data corruption, double-frees, or use-after-free.
--

--- Patch [8]: [PATCH v5 08/10] s390/vfio_ccw: move cp cleanup out of not operational ---
commit a24ecbe23bcd8a2b9448d2b68fd0bbf55921f7a3
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 (by definition) no longer operational. Since this
can happen asynchronously from the normal behavior of the
driver, the cleanup may happen when holding other locks
in the calling sequence (notably, the cio subchannel lock).

Push the cleanup of the private->cp resources to a workqueue,
where it can be done out from under that lock sequence and
(soon) under its own serialization mechanism.

Fixes: 204b394a23ad ("vfio/ccw: Move FSM open/close to MDEV open/close")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Farman <farman@linux.ibm.com>

> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 1a095085bc728..c197ad5ab5805 100644
> --- 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]
This is a pre-existing issue, but moving the cleanup to an asynchronous
workqueue exposes a race condition.

Does calling cp_free() here asynchronously without holding locks race with
other FSM callbacks?

For example, if fsm_close() is triggered concurrently, it calls cp_free()
synchronously outside of sch->lock. Since the cp->initialized check in
cp_free() is non-atomic:

drivers/s390/cio/vfio_ccw_cp.c:cp_free() {
    if (!cp->initialized)
        return;
    cp->initialized = false;
    ...
}

Could multiple threads bypass this check concurrently, leading to double-frees
of the ccwchain_list and use-after-free conditions?

> +}
> +
>  /*
>   * Css driver callbacks
>   */

[ ... ]

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 9048c1e6e7f0c..26c19f0e5e474 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c

[ ... ]

> @@ -138,6 +139,7 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
>  
>  	cancel_work_sync(&private->io_work);
>  	cancel_work_sync(&private->crw_work);
> +	cancel_work_sync(&private->notoper_work);

[Severity: High]
If the device is closed or unbound before the workqueue has a chance to
execute, does canceling notoper_work here silently skip the cleanup of
the channel program resources?

Since fsm_notoper() changes the state to VFIO_CCW_STATE_NOT_OPER when
queuing this work, standard close handlers like fsm_close() bypass
cp_free().

Without a fallback call to cp_free() to reclaim resources if the work
execution was skipped by cancel_work_sync(), will this result in a
permanent memory leak of pinned guest pages and ccwchain structures?

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