[1/2] virtio/s390: avoid race on vcdev->config
diff mbox series

Message ID 20180921124621.43649-2-pasic@linux.ibm.com
State New
Headers show
Series
  • virtio/s390: fix some races in virtio-ccw
Related show

Commit Message

Halil Pasic Sept. 21, 2018, 12:46 p.m. UTC
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(-)

Comments

Cornelia Huck Sept. 21, 2018, 1:14 p.m. UTC | #1
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(-)
Halil Pasic Sept. 21, 2018, 1:28 p.m. UTC | #2
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(-)
>
Farhan Ali Sept. 21, 2018, 9:47 p.m. UTC | #3
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(-)
>>
> 
>
Cornelia Huck Sept. 24, 2018, 12:55 p.m. UTC | #4
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(-)  
> >>  
> > 
> >   
>
Halil Pasic Sept. 24, 2018, 2:27 p.m. UTC | #5
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
Cornelia Huck Sept. 24, 2018, 2:50 p.m. UTC | #6
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.

Patch
diff mbox series

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;