Re: [PATCH v8 09/10] s390/vfio_ccw: selectively expand io_mutex

[Matthew Rosato]

On 7/27/26 9:35 PM, Eric Farman wrote:
> The io_mutex was defined to serialize the io_regions, but then has
> also sort of been associated with the I/O themselves because of
> the close relationship they share.
> 
> With the handful of races that are possible, the choices are either to:
>  A) expand the scope of io_mutex to close these remaining windows, or
>  B) reduce the scope of io_mutex to just io_region, and introduce a new
>     lock mechanism for the remaining I/O resources
> 
> This patch implements A, since B brings with it a lot more interactions
> that would need to be tracked and kept in a correct hierarchy.
> 
> The biggest functional change is the introduction of a workqueue
> element for the cp_free() called out of fsm_notoper(), which is could
> be run in an interrupt context and thus cannot be acquiring mutexes.

Nit: re-word "which is could be run"

Also the wording here kind of implies you added fsm_notoper() in this
patch, but that was last patch - here you just made it use the io_mutex.

Otherwise LGTM.

> 
> Fixes: 4f76617378ee ("vfio-ccw: protect the I/O region")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxx>
> ---
>  drivers/s390/cio/vfio_ccw_chp.c     | 2 +-
>  drivers/s390/cio/vfio_ccw_cp.c      | 8 +++++++-
>  drivers/s390/cio/vfio_ccw_drv.c     | 6 ++++--
>  drivers/s390/cio/vfio_ccw_fsm.c     | 5 +++++
>  drivers/s390/cio/vfio_ccw_private.h | 3 ++-
>  5 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
> index f3015132d4b5..9269b54f5cfd 100644
> --- a/drivers/s390/cio/vfio_ccw_chp.c
> +++ b/drivers/s390/cio/vfio_ccw_chp.c
> @@ -98,13 +98,13 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
>  	if (pos + count > sizeof(*region))
>  		return -EINVAL;
>  
> +	mutex_lock(&private->io_mutex);
>  	crw = list_first_entry_or_null(&private->crw,
>  				       struct vfio_ccw_crw, next);
>  
>  	if (crw)
>  		list_del(&crw->next);
>  
> -	mutex_lock(&private->io_mutex);
>  	if (i >= private->num_regions) {
>  		ret = -EINVAL;
>  		goto out;
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 5ef082b8289a..58722c4baa25 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -977,17 +977,23 @@ void cp_update_scsw(struct channel_program *cp, union scsw *scsw)
>   */
>  bool cp_iova_pinned(struct channel_program *cp, u64 iova, u64 length)
>  {
> +	struct vfio_ccw_private *private =
> +		container_of(cp, struct vfio_ccw_private, cp);
>  	struct ccwchain *chain;
>  	int i;
>  
>  	if (!cp->initialized)
>  		return false;
>  
> +	mutex_lock(&private->io_mutex);
>  	list_for_each_entry(chain, &cp->ccwchain_list, next) {
>  		for (i = 0; i < chain->ch_len; i++)
> -			if (page_array_iova_pinned(&chain->ch_pa[i], iova, length))
> +			if (page_array_iova_pinned(&chain->ch_pa[i], iova, length)) {
> +				mutex_unlock(&private->io_mutex);
>  				return true;
> +			}
>  	}
> +	mutex_unlock(&private->io_mutex);
>  
>  	return false;
>  }
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index c197ad5ab580..757ff5b2556e 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -91,6 +91,7 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
>  
>  	is_final = !(scsw_actl(&irb->scsw) &
>  		     (SCSW_ACTL_DEVACT | SCSW_ACTL_SCHACT));
> +	mutex_lock(&private->io_mutex);
>  	if (scsw_is_solicited(&irb->scsw)) {
>  		cp_update_scsw(&private->cp, &irb->scsw);
>  		if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING) {
> @@ -98,9 +99,7 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
>  			cp_is_finished = true;
>  		}
>  	}
> -	mutex_lock(&private->io_mutex);
>  	memcpy(private->io_region->irb_area, irb, sizeof(*irb));
> -	mutex_unlock(&private->io_mutex);
>  
>  	/*
>  	 * Reset to IDLE only if processing of a channel program
> @@ -110,6 +109,7 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
>  	 */
>  	if (cp_is_finished)
>  		private->state = VFIO_CCW_STATE_IDLE;
> +	mutex_unlock(&private->io_mutex);
>  
>  	if (private->io_trigger)
>  		eventfd_signal(private->io_trigger);
> @@ -131,7 +131,9 @@ void vfio_ccw_notoper_todo(struct work_struct *work)
>  
>  	private = container_of(work, struct vfio_ccw_private, notoper_work);
>  
> +	mutex_lock(&private->io_mutex);
>  	cp_free(&private->cp);
> +	mutex_unlock(&private->io_mutex);
>  }
>  
>  /*
> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> index 4d47a3c7b9a0..5fd94e9d5c61 100644
> --- a/drivers/s390/cio/vfio_ccw_fsm.c
> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> @@ -170,6 +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 routine could be called from IRQ context, so defer */
>  	queue_work(vfio_ccw_work_q, &private->notoper_work);
>  }
>  
> @@ -409,7 +410,11 @@ static void fsm_close(struct vfio_ccw_private *private,
>  
>  	private->state = VFIO_CCW_STATE_STANDBY;
>  	spin_unlock_irq(&sch->lock);
> +
> +	mutex_lock(&private->io_mutex);
>  	cp_free(&private->cp);
> +	mutex_unlock(&private->io_mutex);
> +
>  	return;
>  
>  err_unlock:
> diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
> index e2256402b089..739121116ab6 100644
> --- a/drivers/s390/cio/vfio_ccw_private.h
> +++ b/drivers/s390/cio/vfio_ccw_private.h
> @@ -88,7 +88,8 @@ struct vfio_ccw_parent {
>   * @state: internal state of the device
>   * @completion: synchronization helper of the I/O completion
>   * @io_region: MMIO region to input/output I/O arguments/results
> - * @io_mutex: protect against concurrent update of I/O regions
> + * @io_mutex: protect against concurrent update of I/O resources
> + *            and @cp lifecycle
>   * @region: additional regions for other subchannel operations
>   * @cmd_region: MMIO region for asynchronous I/O commands other than START
>   * @schib_region: MMIO region for SCHIB information