[PULL 0/2] virtio-ccw fixes

[PULL 0/2] virtio-ccw fixes

[Cornelia Huck]

The following changes since commit 8c0f9f5b309d627182d5da72a69246f58bde1026:

  Revert "uapi/linux/keyctl.h: don't use C++ reserved keyword as a struct member name" (2018-09-25 13:28:58 +0200)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/kvms390/linux.git tags/virtio-ccw-20180926

for you to fetch changes up to 1fd7ccf82385b3b14e4b3f009a82ada17ddc1e6f:

  virtio/s390: fix race in ccw_io_helper() (2018-09-25 15:29:10 +0200)

----------------------------------------------------------------
Two patches fixing races in the virtio-ccw driver.

----------------------------------------------------------------

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

-- 
2.14.4

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

[Cornelia Huck]

From: Halil Pasic <pasic@linux.ibm.com>

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

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
Cc: stable@vger.kernel.org
Message-Id: <20180925121309.58524-2-pasic@linux.ibm.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 drivers/s390/virtio/virtio_ccw.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

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;
-- 
2.14.4

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

[Cornelia Huck]

From: Halil Pasic <pasic@linux.ibm.com>

While ccw_io_helper() seems like intended to be exclusive in a sense that
it is supposed to facilitate I/O for at most one thread at any given
time, there is actually nothing ensuring that threads won't pile up at
vcdev->wait_q. If they do, all threads get woken up and see the status
that belongs to some other request than their own. This can lead to bugs.
For an example see:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1788432

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 used quite infrequent
too.

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.

Let us fix the problem by ensuring, that for each device, we finish
processing the previous request before starting with a new one.

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
Reported-by: Colin Ian King <colin.king@canonical.com>
Cc: stable@vger.kernel.org
Message-Id: <20180925121309.58524-3-pasic@linux.ibm.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 drivers/s390/virtio/virtio_ccw.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index a5e8530a3391..b67dc4974f23 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -56,6 +56,7 @@ struct virtio_ccw_device {
 	unsigned int revision; /* Transport revision */
 	wait_queue_head_t wait_q;
 	spinlock_t lock;
+	struct mutex io_lock; /* Serializes I/O requests */
 	struct list_head virtqueues;
 	unsigned long indicators;
 	unsigned long indicators2;
@@ -296,6 +297,7 @@ static int ccw_io_helper(struct virtio_ccw_device *vcdev,
 	unsigned long flags;
 	int flag = intparm & VIRTIO_CCW_INTPARM_MASK;
 
+	mutex_lock(&vcdev->io_lock);
 	do {
 		spin_lock_irqsave(get_ccwdev_lock(vcdev->cdev), flags);
 		ret = ccw_device_start(vcdev->cdev, ccw, intparm, 0, 0);
@@ -308,7 +310,9 @@ static int ccw_io_helper(struct virtio_ccw_device *vcdev,
 		cpu_relax();
 	} while (ret == -EBUSY);
 	wait_event(vcdev->wait_q, doing_io(vcdev, flag) == 0);
-	return ret ? ret : vcdev->err;
+	ret = ret ? ret : vcdev->err;
+	mutex_unlock(&vcdev->io_lock);
+	return ret;
 }
 
 static void virtio_ccw_drop_indicator(struct virtio_ccw_device *vcdev,
@@ -1253,6 +1257,7 @@ static int virtio_ccw_online(struct ccw_device *cdev)
 	init_waitqueue_head(&vcdev->wait_q);
 	INIT_LIST_HEAD(&vcdev->virtqueues);
 	spin_lock_init(&vcdev->lock);
+	mutex_init(&vcdev->io_lock);
 
 	spin_lock_irqsave(get_ccwdev_lock(cdev), flags);
 	dev_set_drvdata(&cdev->dev, vcdev);
-- 
2.14.4

Re: [PULL 0/2] virtio-ccw fixes

[Cornelia Huck]

On Wed, 26 Sep 2018 18:48:28 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> The following changes since commit 8c0f9f5b309d627182d5da72a69246f58bde1026:
> 
>   Revert "uapi/linux/keyctl.h: don't use C++ reserved keyword as a struct member name" (2018-09-25 13:28:58 +0200)
> 
> are available in the Git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/kvms390/linux.git tags/virtio-ccw-20180926
> 
> for you to fetch changes up to 1fd7ccf82385b3b14e4b3f009a82ada17ddc1e6f:
> 
>   virtio/s390: fix race in ccw_io_helper() (2018-09-25 15:29:10 +0200)
> 
> ----------------------------------------------------------------
> Two patches fixing races in the virtio-ccw driver.
> 
> ----------------------------------------------------------------
> 
> 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(-)
> 

Ping. I don't see these patches in master :(

Re: [PULL 0/2] virtio-ccw fixes

[Cornelia Huck]

On Thu, 29 Nov 2018 15:56:48 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, 26 Sep 2018 18:48:28 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > The following changes since commit 8c0f9f5b309d627182d5da72a69246f58bde1026:
> > 
> >   Revert "uapi/linux/keyctl.h: don't use C++ reserved keyword as a struct member name" (2018-09-25 13:28:58 +0200)
> > 
> > are available in the Git repository at:
> > 
> >   git://git.kernel.org/pub/scm/linux/kernel/git/kvms390/linux.git tags/virtio-ccw-20180926
> > 
> > for you to fetch changes up to 1fd7ccf82385b3b14e4b3f009a82ada17ddc1e6f:
> > 
> >   virtio/s390: fix race in ccw_io_helper() (2018-09-25 15:29:10 +0200)
> > 
> > ----------------------------------------------------------------
> > Two patches fixing races in the virtio-ccw driver.
> > 
> > ----------------------------------------------------------------
> > 
> > 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(-)
> >   
> 
> Ping. I don't see these patches in master :(

Ping^2. Any chance to get this into the next version?

Re: [PULL 0/2] virtio-ccw fixes

[Michael S. Tsirkin]

On Thu, Dec 06, 2018 at 01:18:28PM +0100, Cornelia Huck wrote:
> On Thu, 29 Nov 2018 15:56:48 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Wed, 26 Sep 2018 18:48:28 +0200
> > Cornelia Huck <cohuck@redhat.com> wrote:
> > 
> > > The following changes since commit 8c0f9f5b309d627182d5da72a69246f58bde1026:
> > > 
> > >   Revert "uapi/linux/keyctl.h: don't use C++ reserved keyword as a struct member name" (2018-09-25 13:28:58 +0200)
> > > 
> > > are available in the Git repository at:
> > > 
> > >   git://git.kernel.org/pub/scm/linux/kernel/git/kvms390/linux.git tags/virtio-ccw-20180926
> > > 
> > > for you to fetch changes up to 1fd7ccf82385b3b14e4b3f009a82ada17ddc1e6f:
> > > 
> > >   virtio/s390: fix race in ccw_io_helper() (2018-09-25 15:29:10 +0200)
> > > 
> > > ----------------------------------------------------------------
> > > Two patches fixing races in the virtio-ccw driver.
> > > 
> > > ----------------------------------------------------------------
> > > 
> > > 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(-)
> > >   
> > 
> > Ping. I don't see these patches in master :(
> 
> Ping^2. Any chance to get this into the next version?

Pulled, thanks!