[2/2] virtio/s390: fix race in ccw_io_helper()
diff mbox series

Message ID 20180921124621.43649-3-pasic@linux.ibm.com
State New
Headers show
Series
  • virtio/s390: fix some races in virtio-ccw
Related show

Commit Message

Halil Pasic Sept. 21, 2018, 12:46 p.m. UTC
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 all threads get woken up and see the status that
belongs to some other request as their own. This can lead to bugs. For an
example see :
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1788432

This normally does not cause problems, as these are usually infrequent
operations that happen in a well defined sequence and normally do not
fail. But occasionally sysfs attributes are directly dependent on pieces
of virio config and trigger a get on each read.  This gives us at least
one method to trigger races.

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: Halil Pasic <pasic@linux.ibm.com>
Reported-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/s390/virtio/virtio_ccw.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Cornelia Huck Sept. 21, 2018, 1:30 p.m. UTC | #1
On Fri, 21 Sep 2018 14:46:21 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> 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 all threads get woken up and see the status that

s/If they/If they do,/

> belongs to some other request as their own. This can lead to bugs. For an

s/as/than/

> example see :
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1788432
> 
> This normally does not cause problems, as these are usually infrequent
> operations that happen in a well defined sequence and normally do not
> fail. But occasionally sysfs attributes are directly dependent on pieces
> of virio config and trigger a get on each read.  This gives us at least
> one method to trigger races.

Again, I'd prefer a rewording, but a slightly different one:

"These normally does not cause problems, as these are usually
infrequent operations that happen in a well defined sequence and
normally do not fail. But for example, sysfs attributes may trigger
concurrent reading/writing of the config space, including read before
write."

(Not sure if that's better...)

> 
> 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: Halil Pasic <pasic@linux.ibm.com>
> Reported-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/s390/virtio/virtio_ccw.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

Otherwise, looks good.
Halil Pasic Sept. 24, 2018, 12:57 p.m. UTC | #2
On 09/21/2018 03:30 PM, Cornelia Huck wrote:
> On Fri, 21 Sep 2018 14:46:21 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
>> 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 all threads get woken up and see the status that
> 
> s/If they/If they do,/

Nod.

> 
>> belongs to some other request as their own. This can lead to bugs. For an
> 
> s/as/than/

Nod.

> 
>> example see :
>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1788432
>>
>> This normally does not cause problems, as these are usually infrequent
>> operations that happen in a well defined sequence and normally do not
>> fail. But occasionally sysfs attributes are directly dependent on pieces
>> of virio config and trigger a get on each read.  This gives us at least
>> one method to trigger races.
> 
> Again, I'd prefer a rewording, but a slightly different one:
> 
> "These normally does not cause problems, as these are usually
> infrequent operations that happen in a well defined sequence and
> normally do not fail. But for example, sysfs attributes may trigger
> concurrent reading/writing of the config space, including read before
> write."
> 
> (Not sure if that's better...)

Again, not what I tried to say. By 'This' I meant 'this race'. How
about?

"""
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 usually 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.
"""

Halil


> 
>>
>> 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: Halil Pasic <pasic@linux.ibm.com>
>> Reported-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>  drivers/s390/virtio/virtio_ccw.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> Otherwise, looks good.
>
Cornelia Huck Sept. 24, 2018, 1:32 p.m. UTC | #3
On Mon, 24 Sep 2018 14:57:26 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On 09/21/2018 03:30 PM, Cornelia Huck wrote:
> > On Fri, 21 Sep 2018 14:46:21 +0200
> > Halil Pasic <pasic@linux.ibm.com> wrote:

> Again, not what I tried to say. By 'This' I meant 'this race'. How
> about?
> 
> """
> 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 usually quite infrequent too.

s/usually/used/

> 
> 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.
> """

Fine with me.
Halil Pasic Sept. 24, 2018, 2:09 p.m. UTC | #4
On 09/24/2018 03:32 PM, Cornelia Huck wrote:
> On Mon, 24 Sep 2018 14:57:26 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
>> On 09/21/2018 03:30 PM, Cornelia Huck wrote:
>>> On Fri, 21 Sep 2018 14:46:21 +0200
>>> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
>> Again, not what I tried to say. By 'This' I meant 'this race'. How
>> about?
>>
>> """
>> 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 usually quite infrequent too.
> 
> s/usually/used/

Nod. Thanks!

> 
>>
>> 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.
>> """
> 
> Fine with me.
>

Patch
diff mbox series

diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index a5e8530a3391..b67dc4974f23 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -56,6 +56,7 @@  struct virtio_ccw_device {
 	unsigned int revision; /* Transport revision */
 	wait_queue_head_t wait_q;
 	spinlock_t lock;
+	struct mutex io_lock; /* Serializes I/O requests */
 	struct list_head virtqueues;
 	unsigned long indicators;
 	unsigned long indicators2;
@@ -296,6 +297,7 @@  static int ccw_io_helper(struct virtio_ccw_device *vcdev,
 	unsigned long flags;
 	int flag = intparm & VIRTIO_CCW_INTPARM_MASK;
 
+	mutex_lock(&vcdev->io_lock);
 	do {
 		spin_lock_irqsave(get_ccwdev_lock(vcdev->cdev), flags);
 		ret = ccw_device_start(vcdev->cdev, ccw, intparm, 0, 0);
@@ -308,7 +310,9 @@  static int ccw_io_helper(struct virtio_ccw_device *vcdev,
 		cpu_relax();
 	} while (ret == -EBUSY);
 	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,
@@ -1253,6 +1257,7 @@  static int virtio_ccw_online(struct ccw_device *cdev)
 	init_waitqueue_head(&vcdev->wait_q);
 	INIT_LIST_HEAD(&vcdev->virtqueues);
 	spin_lock_init(&vcdev->lock);
+	mutex_init(&vcdev->io_lock);
 
 	spin_lock_irqsave(get_ccwdev_lock(cdev), flags);
 	dev_set_drvdata(&cdev->dev, vcdev);