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

New issues:
- [Critical] Concurrent execution of `cp_free()` via `notoper_work` without synchronization leads to data races and double-free of channel program resources.
- [High] Canceling `notoper_work` during device release or close permanently leaks channel program resources if the work is pending.

Pre-existing issues:
- [Critical] A TOCTOU race in `vfio_ccw_sch_event()` can queue `notoper_work` on a freed `private` structure during concurrent device removal.
--

--- Patch [8]: [PATCH v7 08/10] s390/vfio_ccw: move cp cleanup out of not operational ---
commit 553f8e3a76925556dafb452c43ea6719ddd97d0b
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. Push the cleanup of the private->cp
resources to a workqueue.

Cc: stable@vger.kernel.org

> 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: Critical]
Does this introduce a double-free race condition?  The new workqueue
callback invokes cp_free() asynchronously without acquiring io_mutex or
the subchannel lock.

Concurrently, fsm_close() might also execute and call cp_free(). Since
cp_free() checks and updates the initialized flag locklessly:

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

    cp->initialized = false;
    ...
}

Could both threads proceed to double-free the channel program resources and
double-unpin guest memory pages?

> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> index 4d7988ea47ef0..4d47a3c7b9a06 100644
> --- a/drivers/s390/cio/vfio_ccw_fsm.c
> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> @@ -170,8 +170,7 @@ 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);
> +	queue_work(vfio_ccw_work_q, &private->notoper_work);

[Severity: Critical]
This is a pre-existing issue, but could this queue_work() trigger a
use-after-free due to a race with device removal?

In vfio_ccw_sch_event(), the private structure is fetched locklessly:

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

Concurrently, if userspace initiates device removal, vfio_ccw_mdev_remove()
clears drvdata and frees the private structure via vfio_put_device(). If
vfio_ccw_sch_event() is preempted after fetching the pointer but before
acquiring the lock, could it pass a freed private pointer into
vfio_ccw_fsm_event(), which then queues the notoper_work?

>  }
>  
>  /*
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index bd488e40e1531..8ec6b175d9914 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
[ ... ]
> @@ -139,6 +140,7 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
>  	/* Should be empty, but just in case */
>  	cancel_work_sync(&private->io_work);
>  	cancel_work_sync(&private->crw_work);
> +	cancel_work_sync(&private->notoper_work);

[Severity: High]
Does canceling the deferred cleanup synchronously here leak the channel
program resources?

If the device transitions to VFIO_CCW_STATE_NOT_OPER, queue_work() schedules
this cleanup. If the device is subsequently closed or released before the
workqueue executes, cancel_work_sync() will abort the pending cleanup.

Furthermore, because the state is VFIO_CCW_STATE_NOT_OPER, the normal close
handler is bypassed:

drivers/s390/cio/vfio_ccw_fsm.c:vfio_ccw_jumptable
    [VFIO_CCW_STATE_NOT_OPER][VFIO_CCW_EVENT_CLOSE] = fsm_nop,

Does this completely bypass cp_free() and permanently leak the allocations
and pinned guest memory pages?

>  
>  	kmem_cache_free(vfio_ccw_crw_region, private->crw_region);

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