Re: [PATCH v3 7/8] s390/vfio_ccw: implement a channel program mutex

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Dropping `sch->lock` in `vfio_ccw_sch_event` before dispatching the FSM event causes `css_sched_sch_todo` to be called locklessly, leading to lost unregister events.

Pre-existing issues:
- [High] Missing locking in `vfio_ccw_mdev_reset` allows concurrent `VFIO_DEVICE_RESET` ioctls to trigger physical subchannel unregistration.
--

--- Patch [7]: [PATCH v3 7/8] s390/vfio_ccw: implement a channel program mutex ---
commit 7b4b6e7c580cf97b697d04eaf47e890ff6d29bb0
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: implement a channel program mutex
    
The channel_program struct is manipulated without a serialization
mechanism to ensure consistent behavior. Take a broad stroke of
putting the entire structure behind a mutex, and ensure everything
that needs private->cp holds this mutex.
    
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
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -259,12 +263,16 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
>  	rc = 0;
>  
>  	if (cio_update_schib(sch)) {
> -		if (private)
> +		if (private) {
> +			spin_unlock_irqrestore(&sch->lock, flags);
>  			vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
> +			goto out;
> +		}
>  	}

[Severity: High]
Since sch->lock is dropped here, does this allow css_sched_sch_todo() to be
called locklessly?

If vfio_ccw_fsm_event() routes to fsm_notoper(), we have:

vfio_ccw_sch_event()
  spin_unlock_irqrestore(&sch->lock, flags);
  vfio_ccw_fsm_event() -> fsm_notoper()
    css_sched_sch_todo(sch, SCH_TODO_UNREG);

Could this lead to a race on sch->todo, allowing other events to overwrite
SCH_TODO_UNREG and leaving the kernel state out of sync with hardware?

> 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
> @@ -38,8 +38,13 @@ static void vfio_ccw_dma_unmap(struct vfio_device *vdev, u64 iova, u64 length)
>  		container_of(vdev, struct vfio_ccw_private, vdev);
>  
>  	/* Drivers MUST unpin pages in response to an invalidation. */
> -	if (!cp_iova_pinned(&private->cp, iova, length))
> +	mutex_lock(&private->cp_mutex);
> +	if (!cp_iova_pinned(&private->cp, iova, length)) {
> +		mutex_unlock(&private->cp_mutex);
>  		return;
> +	}
> +
> +	mutex_unlock(&private->cp_mutex);
>  
>  	vfio_ccw_mdev_reset(private);
>  }

[Severity: High]
This is a pre-existing issue, but does vfio_ccw_mdev_reset() lack locking
protection against concurrent invocations?

If two threads concurrently trigger vfio_ccw_mdev_reset() (e.g. via the
VFIO_DEVICE_RESET ioctl), they could interleave:

Thread A issues VFIO_CCW_EVENT_CLOSE, transitioning to STANDBY.
Before Thread A issues OPEN, Thread B issues VFIO_CCW_EVENT_CLOSE.

The FSM receives CLOSE while in STANDBY state, which routes to fsm_notoper()
and schedules SCH_TODO_UNREG. Could this cause the physical subchannel to be
permanently unregistered, acting as a host denial of service?

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