On 09/18/2018 08:45 PM, Cornelia Huck wrote: > On Wed, 12 Sep 2018 16:02:02 +0200 > Halil Pasic <pasic@xxxxxxxxxxxxx> 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 >> 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. > > Yes, the idea behind ccw_io_helper() was to provide a simple way to use > the inherently asynchronous channel I/O operations in a synchronous > way, as that's what the virtio callbacks expect. I did not consider > multiple callbacks for a device running at the same time; but if the > interface allows that, we obviously need to be able to handle it. > > Has this only been observed for the config get/set commands? (The > read-before-write thing?) > >> >> Signed-off-by: Halil Pasic <pasic@xxxxxxxxxxxxx> >> Reported-by: Colin Ian King <colin.king@xxxxxxxxxxxxx> >> --- >> >> This is a big hammer -- mutex on virtio_ccw device level would more than >> suffice. But I don't think it hurts, and maybe there is a better way e.g. >> one using some common ccw/cio mechanisms to address this. That's why this >> is an RFC. > > I'm for using more delicate tools, if possible :) > > We basically have two options: > - Have a way to queue I/O operations and then handle them in sequence. > Creates complexity, and is likely overkill. (We already have a kind > of serialization because we re-submit the channel program until the > hypervisor accepts it; the problem comes from the wait queue usage.) I secretly hoped we already have something like this somewhere. Getting some kind of requests processed and wanting to know if each of these worked or not seemed like fairly common. I agree, implementing this just for virtio-ccw would be an overkill, I agree. > - Add serialization around the submit/wait procedure (as you did), but > with a per-device mutex. That looks like the easiest solution. > Yep, I'm for doing something like this first. We can think about doing something more elaborate later. I will send a non-RFC with an extra per-device mutex. Unless you object. Another thought that crossed my head was making the transport ops mutex on each virtio-ccw device -- see our conversation on get/set config. I don't think it would make a big difference, since the ccw stuff is mutex already, so we only have parallelism for the preparation and for post-processing the results of the ccw io. Regards, Halil >> --- >> drivers/s390/virtio/virtio_ccw.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c >> index a5e8530a3391..36252f344660 100644 >> --- a/drivers/s390/virtio/virtio_ccw.c >> +++ b/drivers/s390/virtio/virtio_ccw.c >> @@ -289,6 +289,8 @@ static int doing_io(struct virtio_ccw_device *vcdev, __u32 flag) >> return ret; >> } >> >> +DEFINE_MUTEX(vcio_mtx); >> + >> static int ccw_io_helper(struct virtio_ccw_device *vcdev, >> struct ccw1 *ccw, __u32 intparm) >> { >> @@ -296,6 +298,7 @@ static int ccw_io_helper(struct virtio_ccw_device *vcdev, >> unsigned long flags; >> int flag = intparm & VIRTIO_CCW_INTPARM_MASK; >> >> + mutex_lock(&vcio_mtx); >> do { >> spin_lock_irqsave(get_ccwdev_lock(vcdev->cdev), flags); >> ret = ccw_device_start(vcdev->cdev, ccw, intparm, 0, 0); >> @@ -308,7 +311,9 @@ static int ccw_io_helper(struct virtio_ccw_device *vcdev, >> cpu_relax(); >> } while (ret == -EBUSY); > > We probably still want to keep this while loop to be on the safe side > (unsolicited status from the hypervisor, for example.) > Nod. >> wait_event(vcdev->wait_q, doing_io(vcdev, flag) == 0); >> - return ret ? ret : vcdev->err; >> + ret = ret ? ret : vcdev->err; >> + mutex_unlock(&vcio_mtx); >> + return ret; >> } >> >> static void virtio_ccw_drop_indicator(struct virtio_ccw_device *vcdev, >
