Re: [PATCH v4 8/9] s390/vfio_ccw: implement a channel program mutex

[Matthew Rosato <mjrosato@linux.ibm.com>]

On 7/25/26 11:27 AM, Eric Farman wrote:
> 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.
> 
> There are a couple where the cio layer's subchannel->lock performs
> this role in this code, which isn't correct (it should only be used
> when touching the actual subchannel, like cio_enable_subchannel()),
> so adjust the locations where that spinlock is acquired/released
> to correctly coexist with this new mutex.
> 
> Fixes: 0a19e61e6d4c ("vfio: ccw: introduce channel program interfaces")
> Cc: stable@vger.kernel.org
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

> ---
>  drivers/s390/cio/vfio_ccw_cp.c      | 30 +++++++++++++++++++++++++----
>  drivers/s390/cio/vfio_ccw_drv.c     |  6 ++++++
>  drivers/s390/cio/vfio_ccw_fsm.c     | 20 ++++++++++++-------
>  drivers/s390/cio/vfio_ccw_ops.c     | 10 +++++++++-
>  drivers/s390/cio/vfio_ccw_private.h |  3 +++
>  5 files changed, 57 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 5ef082b8289a..ab66caff9894 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -738,12 +738,15 @@ static int ccwchain_fetch_one(struct ccw1 *ccw,
>   */
>  int cp_init(struct channel_program *cp, union orb *orb)
>  {
> -	struct vfio_device *vdev =
> -		&container_of(cp, struct vfio_ccw_private, cp)->vdev;
> +	struct vfio_ccw_private *private =
> +		container_of(cp, struct vfio_ccw_private, cp);
> +	struct vfio_device *vdev = &private->vdev;
>  	/* custom ratelimit used to avoid flood during guest IPL */
>  	static DEFINE_RATELIMIT_STATE(ratelimit_state, 5 * HZ, 1);
>  	int ret;
>  
> +	lockdep_assert_held(&private->cp_mutex);
> +
>  	/* this is an error in the caller */
>  	if (cp->initialized)
>  		return -EBUSY;
> @@ -784,11 +787,14 @@ int cp_init(struct channel_program *cp, union orb *orb)
>   */
>  void cp_free(struct channel_program *cp)
>  {
> -	struct vfio_device *vdev =
> -		&container_of(cp, struct vfio_ccw_private, cp)->vdev;
> +	struct vfio_ccw_private *private =
> +		container_of(cp, struct vfio_ccw_private, cp);
> +	struct vfio_device *vdev = &private->vdev;
>  	struct ccwchain *chain, *temp;
>  	int i;
>  
> +	lockdep_assert_held(&private->cp_mutex);
> +
>  	if (!cp->initialized)
>  		return;
>  
> @@ -841,11 +847,15 @@ void cp_free(struct channel_program *cp)
>   */
>  int cp_prefetch(struct channel_program *cp)
>  {
> +	struct vfio_ccw_private *private =
> +		container_of(cp, struct vfio_ccw_private, cp);
>  	struct ccwchain *chain;
>  	struct ccw1 *ccw;
>  	struct page_array *pa;
>  	int len, idx, ret;
>  
> +	lockdep_assert_held(&private->cp_mutex);
> +
>  	/* this is an error in the caller */
>  	if (!cp->initialized)
>  		return -EINVAL;
> @@ -883,10 +893,14 @@ int cp_prefetch(struct channel_program *cp)
>   */
>  union orb *cp_get_orb(struct channel_program *cp, struct subchannel *sch)
>  {
> +	struct vfio_ccw_private *private =
> +		container_of(cp, struct vfio_ccw_private, cp);
>  	union orb *orb;
>  	struct ccwchain *chain;
>  	struct ccw1 *cpa;
>  
> +	lockdep_assert_held(&private->cp_mutex);
> +
>  	/* this is an error in the caller */
>  	if (!cp->initialized)
>  		return NULL;
> @@ -931,10 +945,14 @@ union orb *cp_get_orb(struct channel_program *cp, struct subchannel *sch)
>   */
>  void cp_update_scsw(struct channel_program *cp, union scsw *scsw)
>  {
> +	struct vfio_ccw_private *private =
> +		container_of(cp, struct vfio_ccw_private, cp);
>  	struct ccwchain *chain;
>  	dma32_t cpa = scsw->cmd.cpa;
>  	u32 ccw_head;
>  
> +	lockdep_assert_held(&private->cp_mutex);
> +
>  	if (!cp->initialized)
>  		return;
>  
> @@ -977,9 +995,13 @@ 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;
>  
> +	lockdep_assert_held(&private->cp_mutex);
> +
>  	if (!cp->initialized)
>  		return false;
>  
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index c197ad5ab580..4830f0dd9c3a 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -91,6 +91,8 @@ 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->cp_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,6 +100,8 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
>  			cp_is_finished = true;
>  		}
>  	}
> +	mutex_unlock(&private->cp_mutex);
> +
>  	mutex_lock(&private->io_mutex);
>  	memcpy(private->io_region->irb_area, irb, sizeof(*irb));
>  	mutex_unlock(&private->io_mutex);
> @@ -131,7 +135,9 @@ void vfio_ccw_notoper_todo(struct work_struct *work)
>  
>  	private = container_of(work, struct vfio_ccw_private, notoper_work);
>  
> +	mutex_lock(&private->cp_mutex);
>  	cp_free(&private->cp);
> +	mutex_unlock(&private->cp_mutex);
>  }
>  
>  /*
> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> index 4d47a3c7b9a0..cefdfcb0cad7 100644
> --- a/drivers/s390/cio/vfio_ccw_fsm.c
> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> @@ -25,17 +25,15 @@ static int fsm_io_helper(struct vfio_ccw_private *private)
>  	unsigned long flags;
>  	int ret;
>  
> -	spin_lock_irqsave(&sch->lock, flags);
> -
>  	orb = cp_get_orb(&private->cp, sch);
> -	if (!orb) {
> -		ret = -EIO;
> -		goto out;
> -	}
> +	if (!orb)
> +		return -EIO;
>  
>  	VFIO_CCW_TRACE_EVENT(5, "stIO");
>  	VFIO_CCW_TRACE_EVENT(5, dev_name(&sch->dev));
>  
> +	spin_lock_irqsave(&sch->lock, flags);
> +
>  	/* Issue "Start Subchannel" */
>  	ccode = ssch(sch->schid, orb);
>  
> @@ -71,7 +69,6 @@ static int fsm_io_helper(struct vfio_ccw_private *private)
>  	default:
>  		ret = ccode;
>  	}
> -out:
>  	spin_unlock_irqrestore(&sch->lock, flags);
>  	return ret;
>  }
> @@ -251,6 +248,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
>  	private->state = VFIO_CCW_STATE_CP_PROCESSING;
>  	memcpy(scsw, io_region->scsw_area, sizeof(*scsw));
>  
> +	mutex_lock(&private->cp_mutex);
> +
>  	if (scsw->cmd.fctl & SCSW_FCTL_START_FUNC) {
>  		orb = (union orb *)io_region->orb_area;
>  
> @@ -299,6 +298,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
>  			cp_free(&private->cp);
>  			goto err_out;
>  		}
> +
> +		mutex_unlock(&private->cp_mutex);
>  		return;
>  	} else if (scsw->cmd.fctl & SCSW_FCTL_HALT_FUNC) {
>  		VFIO_CCW_MSG_EVENT(2,
> @@ -319,6 +320,7 @@ static void fsm_io_request(struct vfio_ccw_private *private,
>  	}
>  
>  err_out:
> +	mutex_unlock(&private->cp_mutex);
>  	private->state = VFIO_CCW_STATE_IDLE;
>  	trace_vfio_ccw_fsm_io_request(scsw->cmd.fctl, schid,
>  				      io_region->ret_code, errstr);
> @@ -409,7 +411,11 @@ static void fsm_close(struct vfio_ccw_private *private,
>  
>  	private->state = VFIO_CCW_STATE_STANDBY;
>  	spin_unlock_irq(&sch->lock);
> +
> +	mutex_lock(&private->cp_mutex);
>  	cp_free(&private->cp);
> +	mutex_unlock(&private->cp_mutex);
> +
>  	return;
>  
>  err_unlock:
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 6c74d596be9d..9242a37677a0 100644
> --- 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);
>  }
> @@ -50,6 +55,7 @@ static int vfio_ccw_mdev_init_dev(struct vfio_device *vdev)
>  		container_of(vdev, struct vfio_ccw_private, vdev);
>  
>  	mutex_init(&private->io_mutex);
> +	mutex_init(&private->cp_mutex);
>  	private->state = VFIO_CCW_STATE_STANDBY;
>  	INIT_LIST_HEAD(&private->crw);
>  	INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
> @@ -91,6 +97,7 @@ static int vfio_ccw_mdev_init_dev(struct vfio_device *vdev)
>  out_free_cp:
>  	kfree(private->cp.guest_cp);
>  out_free_private:
> +	mutex_destroy(&private->cp_mutex);
>  	mutex_destroy(&private->io_mutex);
>  	return -ENOMEM;
>  }
> @@ -142,6 +149,7 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
>  	kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
>  	kmem_cache_free(vfio_ccw_io_region, private->io_region);
>  	kfree(private->cp.guest_cp);
> +	mutex_destroy(&private->cp_mutex);
>  	mutex_destroy(&private->io_mutex);
>  }
>  
> diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
> index e2256402b089..b595fd81f370 100644
> --- a/drivers/s390/cio/vfio_ccw_private.h
> +++ b/drivers/s390/cio/vfio_ccw_private.h
> @@ -94,6 +94,7 @@ struct vfio_ccw_parent {
>   * @schib_region: MMIO region for SCHIB information
>   * @crw_region: MMIO region for getting channel report words
>   * @num_regions: number of additional regions
> + * @cp_mutex: protect against concurrent update of CP resources
>   * @cp: channel program for the current I/O operation
>   * @irb: irb info received from interrupt
>   * @scsw: scsw info
> @@ -116,7 +117,9 @@ struct vfio_ccw_private {
>  	struct ccw_crw_region	*crw_region;
>  	int num_regions;
>  
> +	struct mutex		cp_mutex;
>  	struct channel_program	cp;
> +
>  	struct irb		irb;
>  	union scsw		scsw;
>  	struct list_head	crw;