Re: [PATCH v4 7/9] 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] The newly introduced `notoper_work` is queued asynchronously but never cancelled or flushed when the device is released, leading to a Use-After-Free.
- [High] The asynchronous execution of `cp_free()` in `vfio_ccw_notoper_todo()` runs without any locks, creating a critical race condition with concurrent callers.
--

--- Patch [7]: [PATCH v4 7/9] s390/vfio_ccw: move cp cleanup out of not operational ---
commit fa524c6f90fe876d05c7a596474d882a3c80e457
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: move cp cleanup out of not operational

This commit shifts the cleanup of channel program resources out of the
synchronous fsm_notoper state handler. It defers the cp_free operation
to a workqueue, avoiding potential lock contention with the cio subchannel
lock.

> 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]
Does this asynchronous execution of cp_free need some form of locking?

If userspace closes the device file at the same time this work executes,
fsm_close might call cp_free() concurrently. Since cp_free() checks and
clears cp->initialized non-atomically:

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

Could both threads observe cp->initialized as true, leading to a
double-free of the chain structures? 

The commit message mentions this will be done "(soon) under its own
serialization mechanism", but should that be included in this patch to
prevent the race condition entirely?

> 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: High]
If this work is queued, but the device is subsequently removed and released
via vfio_ccw_mdev_release_dev(), will the work item still be pending?

Since vfio_ccw_mdev_release_dev() frees the private structure, is there a
risk of a use-after-free if the workqueue thread eventually executes
vfio_ccw_notoper_todo() and dereferences the freed private pointer?

Should cancel_work_sync(&private->notoper_work) or flush_work() be added to
the device teardown path to ensure the work is safely stopped before private
is freed?

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