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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
---
drivers/s390/virtio/virtio_ccw.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
