On 09/21/2018 03:30 PM, Cornelia Huck wrote: > On Fri, 21 Sep 2018 14:46:21 +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 > > 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@xxxxxxxxxxxxx> >> Reported-by: Colin Ian King <colin.king@xxxxxxxxxxxxx> >> --- >> drivers/s390/virtio/virtio_ccw.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) > > Otherwise, looks good. >
