| Message ID | 20180921124621.43649-3-pasic@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio/s390: fix some races in virtio-ccw | expand |
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.
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. >
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.
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. >
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);
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(-)