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
>>
>> 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(-)
>
