s390/vfio_ccw: Selectively expand io_mutex

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. It also
takes advantage of the workqueue element for cp_free() that now gets
called out of fsm_notoper(), which could be invoked out of an interrupt
context and thus cannot acquire a mutex itself.

Fixes: 4f76617378ee ("vfio-ccw: protect the I/O region")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>

diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
index f3015132d4b5f768f7b2ad4eaacb33a7b8a9b550..9269b54f5cfd4f8b471d5aa103c8959e5e331697 100644 (file)
--- 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 5ef082b8289af0a2abd408216d086769afe2c52e..58722c4baa251f738df66c7ca6d25eb6567a03a8 100644 (file)
--- 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 c197ad5ab5805674bc124323eaafbc3e5854a0a1..757ff5b2556e33ccc89f05dbafe73cb023198755 100644 (file)
--- 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 4d47a3c7b9a069d907fcaf1ee0284769cfe15bc6..5fd94e9d5c61818f66d517929df146fb2125ceef 100644 (file)
--- 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 e2256402b089409fd4acf1d4a80966d9e4efe08c..739121116ab64c930fd6e6e11b2046171db04a4b 100644 (file)
--- 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