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

[Eric Farman]

On 7/24/26 4:18 PM, Matthew Rosato wrote:
> On 7/23/26 1:47 PM, 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@xxxxxxxxxxxxxxx
> > Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxx>
> > ---
> >   drivers/s390/cio/vfio_ccw_drv.c     | 10 +++++++++-
> >   drivers/s390/cio/vfio_ccw_fsm.c     | 22 +++++++++++++++-------
> >   drivers/s390/cio/vfio_ccw_ops.c     | 10 +++++++++-
> >   drivers/s390/cio/vfio_ccw_private.h |  3 +++
> >   4 files changed, 36 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> > index 1a095085bc72..1d8c2ed9da50 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);
> > @@ -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;
> > +		}
> >   	}
> >   
> >   out_unlock:
> >   	spin_unlock_irqrestore(&sch->lock, flags);
> > +out:
> >   
> >   	return rc;
> >   }
> > diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> > index 4d7988ea47ef..9fbe97bd23f9 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);
>
> Can you extend the documentation for cp_get_orb to indicate that
> cp_mutex must be held on entry?
>
> Actually, better yet, can you use lockdep_assert_held()?

Of course; should've added that in the first place.

> Would be a good idea to put these also in other functions that have now
> an implicit expectation that the mutex will be held, here's the list I
> came up with?
>
> cp_get_orb
> cp_init
> cp_prefetch
> cp_free
> fsm_io_helper
> cp_iova_pinned	
> cp_update_scsw

I'm going to add them to cp_* functions, which means no fsm_io_helper. 
It's only interaction is for calling cp_get_orb anyway (and is the only 
one to do so), so it'll get covered in that path.

> In some cases (like cp_get_orb) you'd have to do something like:
>      struct vfio_ccw_private *private = container_of(cp, struct vfio_ccw_private, cp);
>      lockdep_assert_held(&private->cp_mutex);
>
> Then this lets us also use lockdep to make sure the mutex is OK now
> and in the future.
>
> > -	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;
> >   }
> > @@ -171,7 +168,9 @@ static void fsm_notoper(struct vfio_ccw_private *private,
> >   	private->state = VFIO_CCW_STATE_NOT_OPER;
> >   
> >   	/* This is usually handled during CLOSE event */
> > +	mutex_lock(&private->cp_mutex);
> >   	cp_free(&private->cp);
> > +	mutex_unlock(&private->cp_mutex);
> >   }
> >   
> >   /*
> > @@ -252,6 +251,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;
> >   
> > @@ -300,6 +301,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,
> > @@ -320,6 +323,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);
> > @@ -410,7 +414,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 032a1cdf4df7..04800cfa779b 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);
> > @@ -90,6 +96,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;
> >   }
> > @@ -141,6 +148,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 0501d4bbcdbd..ac5aaa78a74b 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
> > @@ -115,7 +116,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;