| Message ID | 20180921124621.43649-2-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:20 +0200 Halil Pasic <pasic@linux.ibm.com> wrote: > Currently we have a race on vcdev->config in virtio_ccw_get_config() and > in virtio_ccw_set_config(). > > This normally does not cause problems, as these are usually infrequent > operations. 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 trigger. I find this paragraph to be a bit confusing. What about the following: "This normally does not cause problems, as these are usually infrequent operations. However, writing to the config space may be preceded by a read, and different threads may perform read/write operations concurrently, triggered through sysfs attributes." > > Let us protect the shared state using vcdev->lock. > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > --- > drivers/s390/virtio/virtio_ccw.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-)
On 09/21/2018 03:14 PM, Cornelia Huck wrote: > On Fri, 21 Sep 2018 14:46:20 +0200 > Halil Pasic <pasic@linux.ibm.com> wrote: > >> Currently we have a race on vcdev->config in virtio_ccw_get_config() and >> in virtio_ccw_set_config(). >> >> This normally does not cause problems, as these are usually infrequent >> operations. 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 trigger. > > I find this paragraph to be a bit confusing. What about the following: > > "This normally does not cause problems, as these are usually infrequent > operations. However, writing to the config space may be preceded by a > read, and different threads may perform read/write operations > concurrently, triggered through sysfs attributes." > That is not what I was trying to say but it certainly reads nicer. What I was trying to say is: it is unlikely to happen with usual/normal usage. But since static ssize_t virtblk_cache_type_show(struct device *dev, struct device_attribute *attr, char *buf) { struct gendisk *disk = dev_to_disk(dev); struct virtio_blk *vblk = disk->private_data; u8 writeback = virtblk_get_cache_mode(vblk->vdev); BUG_ON(writeback >= ARRAY_SIZE(virtblk_cache_types)); return snprintf(buf, 40, "%s\n", virtblk_cache_types[writeback]); } and static ssize_t status_show(struct device *_d, struct device_attribute *attr, char *buf) { struct virtio_device *dev = dev_to_virtio(_d); return sprintf(buf, "0x%08x\n", dev->config->get_status(dev)); } static DEVICE_ATTR_RO(status); user space can make these into frequent operations. Does user space has a need to poll on these attributes in a tight loop. Probably not. But it certainly can. And it's a valid test. The paragraph was intended to explain how bad the problem actually is, and not to provide further explanation on why is this not correctly synchronized (i.e. racy). Anyway I'm fine with swapping the old out and your new version in, if you prefer it that way. If you do, would you like to have a respin? Regards, Halil >> >> Let us protect the shared state using vcdev->lock. >> >> Signed-off-by: Halil Pasic <pasic@linux.ibm.com> >> --- >> drivers/s390/virtio/virtio_ccw.c | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >
On 09/21/2018 09:28 AM, Halil Pasic wrote: > > > On 09/21/2018 03:14 PM, Cornelia Huck wrote: >> On Fri, 21 Sep 2018 14:46:20 +0200 >> Halil Pasic <pasic@linux.ibm.com> wrote: >> >>> Currently we have a race on vcdev->config in virtio_ccw_get_config() and >>> in virtio_ccw_set_config(). >>> >>> This normally does not cause problems, as these are usually infrequent >>> operations. 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 trigger. >> >> I find this paragraph to be a bit confusing. What about the following: >> >> "This normally does not cause problems, as these are usually infrequent >> operations. However, writing to the config space may be preceded by a >> read, and different threads may perform read/write operations >> concurrently, triggered through sysfs attributes." >> > > That is not what I was trying to say but it certainly reads nicer. > > What I was trying to say is: it is unlikely to happen with usual/normal > usage. But since > > static ssize_t > virtblk_cache_type_show(struct device *dev, struct device_attribute *attr, > char *buf) > { > struct gendisk *disk = dev_to_disk(dev); > struct virtio_blk *vblk = disk->private_data; > u8 writeback = virtblk_get_cache_mode(vblk->vdev); > > BUG_ON(writeback >= ARRAY_SIZE(virtblk_cache_types)); > return snprintf(buf, 40, "%s\n", virtblk_cache_types[writeback]); > } > > and > > static ssize_t status_show(struct device *_d, > struct device_attribute *attr, char *buf) > { > struct virtio_device *dev = dev_to_virtio(_d); > return sprintf(buf, "0x%08x\n", dev->config->get_status(dev)); > } > static DEVICE_ATTR_RO(status); > > user space can make these into frequent operations. > > Does user space has a need to poll on these attributes in a tight > loop. Probably not. But it certainly can. And it's a valid test. > > The paragraph was intended to explain how bad the problem actually > is, and not to provide further explanation on why is this not > correctly synchronized (i.e. racy). > > Anyway I'm fine with swapping the old out and your new version in, > if you prefer it that way. > > If you do, would you like to have a respin? > > Regards, > Halil > I had been looking into this code recently, and shouldn't vcdev->status (function get/set_status functions) also have a lock around it? Or is it not possible to have a race condition on vcdev->status? Thanks Farhan >>> >>> Let us protect the shared state using vcdev->lock. >>> >>> Signed-off-by: Halil Pasic <pasic@linux.ibm.com> >>> --- >>> drivers/s390/virtio/virtio_ccw.c | 10 ++++++++-- >>> 1 file changed, 8 insertions(+), 2 deletions(-) >> > >
On Fri, 21 Sep 2018 17:47:47 -0400 Farhan Ali <alifm@linux.ibm.com> wrote: > On 09/21/2018 09:28 AM, Halil Pasic wrote: > > Anyway I'm fine with swapping the old out and your new version in, > > if you prefer it that way. > > > > If you do, would you like to have a respin? Just send me a respin with something that you find useful :) (maybe a mashup of our descriptions) (while at it, you could also add the cc:stable, which I agree make sense) > > > > Regards, > > Halil > > > > I had been looking into this code recently, and shouldn't vcdev->status > (function get/set_status functions) also have a lock around it? Or is it > not possible to have a race condition on vcdev->status? I don't think so, as status is only a byte. > > Thanks > Farhan > > > >>> > >>> Let us protect the shared state using vcdev->lock. > >>> > >>> Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > >>> --- > >>> drivers/s390/virtio/virtio_ccw.c | 10 ++++++++-- > >>> 1 file changed, 8 insertions(+), 2 deletions(-) > >> > > > > >
On 09/24/2018 02:55 PM, Cornelia Huck wrote: > On Fri, 21 Sep 2018 17:47:47 -0400 > Farhan Ali <alifm@linux.ibm.com> wrote: > >> On 09/21/2018 09:28 AM, Halil Pasic wrote: > >>> Anyway I'm fine with swapping the old out and your new version in, >>> if you prefer it that way. >>> >>> If you do, would you like to have a respin? > > Just send me a respin with something that you find useful :) (maybe a > mashup of our descriptions) (while at it, you could also add the > cc:stable, which I agree make sense) > Will do! I would like to go with. """ This normally does not cause problems, as these are usually infrequent operations. However, for some devices writing to/reading from the config space can be triggered through sysfs attributes. For these userspace can force the race by increasing the frequency. """ >>> >>> Regards, >>> Halil >>> >> >> I had been looking into this code recently, and shouldn't vcdev->status >> (function get/set_status functions) also have a lock around it? Or is it >> not possible to have a race condition on vcdev->status? > > I don't think so, as status is only a byte. > Nod. Thanks, Halil
On Mon, 24 Sep 2018 16:27:30 +0200 Halil Pasic <pasic@linux.ibm.com> wrote: > On 09/24/2018 02:55 PM, Cornelia Huck wrote: > > On Fri, 21 Sep 2018 17:47:47 -0400 > > Farhan Ali <alifm@linux.ibm.com> wrote: > > > >> On 09/21/2018 09:28 AM, Halil Pasic wrote: > > > >>> Anyway I'm fine with swapping the old out and your new version in, > >>> if you prefer it that way. > >>> > >>> If you do, would you like to have a respin? > > > > Just send me a respin with something that you find useful :) (maybe a > > mashup of our descriptions) (while at it, you could also add the > > cc:stable, which I agree make sense) > > > > Will do! I would like to go with. > > > """ > This normally does not cause problems, as these are usually infrequent > operations. However, for some devices writing to/reading from the config space > can be triggered through sysfs attributes. For these userspace can force the > race by increasing the frequency. > """ Sounds good.
diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c index 8f5c1d7f751a..a5e8530a3391 100644 --- a/drivers/s390/virtio/virtio_ccw.c +++ b/drivers/s390/virtio/virtio_ccw.c @@ -828,6 +828,7 @@ static void virtio_ccw_get_config(struct virtio_device *vdev, int ret; struct ccw1 *ccw; void *config_area; + unsigned long flags; ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL); if (!ccw) @@ -846,11 +847,13 @@ static void virtio_ccw_get_config(struct virtio_device *vdev, if (ret) goto out_free; + spin_lock_irqsave(&vcdev->lock, flags); memcpy(vcdev->config, config_area, offset + len); - if (buf) - memcpy(buf, &vcdev->config[offset], len); if (vcdev->config_ready < offset + len) vcdev->config_ready = offset + len; + spin_unlock_irqrestore(&vcdev->lock, flags); + if (buf) + memcpy(buf, config_area + offset, len); out_free: kfree(config_area); @@ -864,6 +867,7 @@ static void virtio_ccw_set_config(struct virtio_device *vdev, struct virtio_ccw_device *vcdev = to_vc_device(vdev); struct ccw1 *ccw; void *config_area; + unsigned long flags; ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL); if (!ccw) @@ -876,9 +880,11 @@ static void virtio_ccw_set_config(struct virtio_device *vdev, /* Make sure we don't overwrite fields. */ if (vcdev->config_ready < offset) virtio_ccw_get_config(vdev, 0, NULL, offset); + spin_lock_irqsave(&vcdev->lock, flags); memcpy(&vcdev->config[offset], buf, len); /* Write the config area to the host. */ memcpy(config_area, vcdev->config, sizeof(vcdev->config)); + spin_unlock_irqrestore(&vcdev->lock, flags); ccw->cmd_code = CCW_CMD_WRITE_CONF; ccw->flags = 0; ccw->count = offset + len;
Currently we have a race on vcdev->config in virtio_ccw_get_config() and in virtio_ccw_set_config(). This normally does not cause problems, as these are usually infrequent operations. 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 trigger. Let us protect the shared state using vcdev->lock. Signed-off-by: Halil Pasic <pasic@linux.ibm.com> --- drivers/s390/virtio/virtio_ccw.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)