Re: [PATCH 0/2] virtio/s390: fix some races in virtio-ccw

Re: [PATCH 0/2] virtio/s390: fix some races in virtio-ccw

[Christian Borntraeger]

Can you consider adding stable tags to the (final version of the) fixes?

On 09/21/2018 02:46 PM, Halil Pasic wrote:
> The trigger for the series is this bug report:
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1788432
> 
> Changelog:
> RFC -> v1:
> * do mutual exclusion on a per device basis in ccw_io_helper() 
> 
> Halil Pasic (2):
>   virtio/s390: avoid race on vcdev->config
>   virtio/s390: fix race in ccw_io_helper()
> 
>  drivers/s390/virtio/virtio_ccw.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 

Re: [PATCH 1/2] virtio/s390: avoid race on vcdev->config

[Cornelia Huck]

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

Re: [PATCH 1/2] virtio/s390: avoid race on vcdev->config

[Cornelia Huck]

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.

Re: [PATCH 2/2] virtio/s390: fix race in ccw_io_helper()

[Cornelia Huck]

On Mon, 24 Sep 2018 14:57:26 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On 09/21/2018 03:30 PM, Cornelia Huck wrote:
> > On Fri, 21 Sep 2018 14:46:21 +0200
> > Halil Pasic <pasic@linux.ibm.com> wrote:

> 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.

s/usually/used/

> 
> 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.
> """

Fine with me.