Re: [PATCH v9 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 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The `vfio_ccw_notoper_todo` work function calls `cp_free()` without acquiring `private->io_mutex`, leading to a severe data race with userspace I/O request processing.
- [High] Asynchronous subchannel events can queue `notoper_work` after `flush_work()` has completed during device release, leading to a use-after-free of the `private` structure.
- [High] Concurrent execution of `cp_free()` from `fsm_close()` and the new `notoper_work` leads to a double free of channel program resources.
--

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

s390/vfio_ccw: move cp cleanup out of not operational

This commit moves the cleanup of the channel program out of the
fsm_notoper() routine and defers it to a workqueue. This avoids
performing the cleanup while holding the cio subchannel lock.

> 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: Critical]
This isn't a bug introduced by this patch, but does moving the cleanup
to a workqueue create a data race with userspace I/O requests?

If vfio_ccw_notoper_todo() runs concurrently with a guest operating system
issuing an I/O request via vfio_ccw_mdev_write_io_region() (which acquires
private->io_mutex), might cp_free() execute locklessly?

If notoper_todo executes during fsm_io_request() processing cp_prefetch(),
it could tear down the channel program and free ccwchain pointers while
they are actively being iterated and dereferenced.

The commit message notes a future patch manages locking, but should this
be protected by io_mutex here to avoid memory leaks and use-after-free
conditions?

[Severity: High]
This is also a pre-existing issue, but could this lockless execution lead
to a double free if the device is closed concurrently?

When the device is closed, fsm_close() drops sch->lock and calls cp_free().
If a notoper event occurs simultaneously after the lock is dropped,
vfio_ccw_sch_event() queues notoper_work.

Since cp_free() is lockless and cp->initialized is not atomically updated,
could both the fsm_close() thread and the new workqueue thread read
cp->initialized as true and double-free the channel program allocations?

> 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

[ ... ]

> @@ -134,9 +135,16 @@ 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.
> +	 *
> +	 * notoper_work should have nothing to do here, because only
> +	 * open devices could have channel_program resources in use
> +	 * and those would be released during close. Nevertheless,
> +	 * call flush here as well to be certain anything that was
> +	 * allocated is freed.
>  	 */
>  	cancel_work_sync(&private->io_work);
>  	cancel_work_sync(&private->crw_work);
> +	flush_work(&private->notoper_work);

[Severity: High]
This is a pre-existing issue, but does flushing the work here guarantee
that new work cannot be queued afterwards?

If a user removes the mdev via sysfs concurrently with a subchannel event,
vfio_ccw_sch_event() fetches the private pointer from drvdata outside
the subchannel lock:

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_sch_event() {
	struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
	...
	if (cio_update_schib(sch)) {
		if (private)
			vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
	}
}

If the event thread is preempted, vfio_ccw_mdev_remove() can complete
unregistering the device and invoke vfio_ccw_mdev_release_dev(). Once
release finishes, flush_work() returns and the private structure is freed.

When the preempted vfio_ccw_sch_event() thread resumes, won't it use the
freed private pointer to call fsm_notoper() and execute queue_work() on
the freed private->notoper_work memory?

Calling queue_work() after the workqueue shutdown sequence completes
might corrupt the workqueue linked list.

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