Commit 78b1a52e authored by Halil Pasic's avatar Halil Pasic Committed by Michael S. Tsirkin

virtio/s390: fix race in ccw_io_helper()

While ccw_io_helper() seems like intended to be exclusive in a sense that
it is supposed to facilitate I/O for at most one thread at any given
time, there is actually nothing ensuring that threads won't pile up at
vcdev->wait_q. If they do, all threads get woken up and see the status
that belongs to some other request than their own. This can lead to bugs.
For an example see:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1788432

This race normally does not cause any problems. The operations provided
by struct virtio_config_ops are usually invoked in a well defined
sequence, normally don't fail, and are normally used quite infrequent
too.

Yet, if some of the these operations are directly triggered via sysfs
attributes, like in the case described by the referenced bug, userspace
is given an opportunity to force races by increasing the frequency of the
given operations.

Let us fix the problem by ensuring, that for each device, we finish
processing the previous request before starting with a new one.
Signed-off-by: default avatarHalil Pasic <pasic@linux.ibm.com>
Reported-by: default avatarColin Ian King <colin.king@canonical.com>
Cc: stable@vger.kernel.org
Message-Id: <20180925121309.58524-3-pasic@linux.ibm.com>
Signed-off-by: default avatarCornelia Huck <cohuck@redhat.com>
Signed-off-by: default avatarMichael S. Tsirkin <mst@redhat.com>
parent 2448a299
...@@ -56,6 +56,7 @@ struct virtio_ccw_device { ...@@ -56,6 +56,7 @@ struct virtio_ccw_device {
unsigned int revision; /* Transport revision */ unsigned int revision; /* Transport revision */
wait_queue_head_t wait_q; wait_queue_head_t wait_q;
spinlock_t lock; spinlock_t lock;
struct mutex io_lock; /* Serializes I/O requests */
struct list_head virtqueues; struct list_head virtqueues;
unsigned long indicators; unsigned long indicators;
unsigned long indicators2; unsigned long indicators2;
...@@ -296,6 +297,7 @@ static int ccw_io_helper(struct virtio_ccw_device *vcdev, ...@@ -296,6 +297,7 @@ static int ccw_io_helper(struct virtio_ccw_device *vcdev,
unsigned long flags; unsigned long flags;
int flag = intparm & VIRTIO_CCW_INTPARM_MASK; int flag = intparm & VIRTIO_CCW_INTPARM_MASK;
mutex_lock(&vcdev->io_lock);
do { do {
spin_lock_irqsave(get_ccwdev_lock(vcdev->cdev), flags); spin_lock_irqsave(get_ccwdev_lock(vcdev->cdev), flags);
ret = ccw_device_start(vcdev->cdev, ccw, intparm, 0, 0); ret = ccw_device_start(vcdev->cdev, ccw, intparm, 0, 0);
...@@ -308,7 +310,9 @@ static int ccw_io_helper(struct virtio_ccw_device *vcdev, ...@@ -308,7 +310,9 @@ static int ccw_io_helper(struct virtio_ccw_device *vcdev,
cpu_relax(); cpu_relax();
} while (ret == -EBUSY); } while (ret == -EBUSY);
wait_event(vcdev->wait_q, doing_io(vcdev, flag) == 0); wait_event(vcdev->wait_q, doing_io(vcdev, flag) == 0);
return ret ? ret : vcdev->err; ret = ret ? ret : vcdev->err;
mutex_unlock(&vcdev->io_lock);
return ret;
} }
static void virtio_ccw_drop_indicator(struct virtio_ccw_device *vcdev, static void virtio_ccw_drop_indicator(struct virtio_ccw_device *vcdev,
...@@ -1253,6 +1257,7 @@ static int virtio_ccw_online(struct ccw_device *cdev) ...@@ -1253,6 +1257,7 @@ static int virtio_ccw_online(struct ccw_device *cdev)
init_waitqueue_head(&vcdev->wait_q); init_waitqueue_head(&vcdev->wait_q);
INIT_LIST_HEAD(&vcdev->virtqueues); INIT_LIST_HEAD(&vcdev->virtqueues);
spin_lock_init(&vcdev->lock); spin_lock_init(&vcdev->lock);
mutex_init(&vcdev->io_lock);
spin_lock_irqsave(get_ccwdev_lock(cdev), flags); spin_lock_irqsave(get_ccwdev_lock(cdev), flags);
dev_set_drvdata(&cdev->dev, vcdev); dev_set_drvdata(&cdev->dev, vcdev);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment