Fix various issue of vhost

PATCH Superseded Jason Wang 3 participants 11 messages Dec 13, 2018

Subsystems (5)

Networking Network drivers KVM virtio core +1 more subsystems

lore

Share & tools

lore

Jason Wang

author

2018-12-13 02:53 UTC #0 lore raw

v3 cover Fix various issue of vhost

Hi:

This series tries to fix various issues of vhost:

- Patch 1 adds a missing write barrier between used idx updating and logging.
- Patch 2-3 brings back the protection of device IOTLB through vq mutex, this fixes possible use after free in device IOTLB entries.

Please consider them for -stable.

Thanks

Changes from V2:
- drop dirty page fix and make it for net-next
Changes from V1:
- silent compiler warning for 32bit.
- use mutex_trylock() on slowpath instead of mutex_lock() even on fast path.

Jason Wang (3):
vhost: make sure used idx is seen before log in vhost_add_used_n()
vhost_net: switch to use mutex_trylock() in vhost_net_busy_poll() Revert "net: vhost: lock the vqs one by one"

 drivers/vhost/net.c   |  8 +++++++-
 drivers/vhost/vhost.c | 23 +++++++++++++++++++----
 2 files changed, 26 insertions(+), 5 deletions(-)

Signature · 1 line

2.17.1

Jason Wang

author

2018-12-13 02:53 UTC #1 lore raw

PATCH v3 1/3 vhost: make sure used idx is seen before log in vhost_add_used_n()

We miss a write barrier that guarantees used idx is updated and seen before log. This will let userspace sync and copy used ring before used idx is update. Fix this by adding a barrier before log_write().

Fixes 8dd014adfea6f ("vhost-net: mergeable buffers support")

Acked-by Michael S. Tsirkin <[email protected]>

Signed-off-by Jason Wang <[email protected]>

---
 drivers/vhost/vhost.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/vhost/vhost.c +2 -0

index 6b98d8e3a5bf..5915f240275a 100644
@@ -2220,6 +2220,8 @@ int vhost_add_used_n(struct vhost_virtqueue *vq, struct vring_used_elem *heads,
 		return -EFAULT;
 	}
 	if (unlikely(vq->log_used)) {
+		/* Make sure used idx is seen before log. */
+		smp_wmb();
 		/* Log used index update. */
 		log_write(vq->log_base,
 			  vq->log_addr + offsetof(struct vring_used, idx),

Signature · 1 line

2.17.1

Jason Wang

author

2018-12-13 02:53 UTC #2 lore raw

PATCH v3 1/3 vhost: make sure used idx is seen before log in vhost_add_used_n()

In reply to Jason Wang

Fixes 8dd014adfea6f ("vhost-net: mergeable buffers support")

Acked-by Michael S. Tsirkin <[email protected]>

Signed-off-by Jason Wang <[email protected]>

---
 drivers/vhost/vhost.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/vhost/vhost.c +2 -0

index 6b98d8e3a5bf..5915f240275a 100644
@@ -2220,6 +2220,8 @@ int vhost_add_used_n(struct vhost_virtqueue *vq, struct vring_used_elem *heads,
 		return -EFAULT;
 	}
 	if (unlikely(vq->log_used)) {
+		/* Make sure used idx is seen before log. */
+		smp_wmb();
 		/* Log used index update. */
 		log_write(vq->log_base,
 			  vq->log_addr + offsetof(struct vring_used, idx),

Signature · 1 line

2.17.1

Jason Wang

author

2018-12-13 02:53 UTC #3 lore raw

PATCH v3 2/3 vhost_net: switch to use mutex_trylock() in vhost_net_busy_poll()

In reply to Jason Wang

We used to hold the mutex of paired virtqueue in vhost_net_busy_poll(). But this will results an inconsistent lock order which may cause deadlock if we try to bring back the protection of device IOTLB with vq mutex that requires to hold mutex of all virtqueues at the same time.

Fix this simply by switching to use mutex_trylock(), when fail just skip the busy polling. This can happen when device IOTLB is under updating which should be rare.

Fixes commit 78139c94dc8c ("net: vhost: lock the vqs one by one")

Cc Tonghao Zhang <[email protected]>

Acked-by Michael S. Tsirkin <[email protected]>

Signed-off-by Jason Wang <[email protected]>

---
 drivers/vhost/net.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/vhost/net.c +7 -1

index ab11b2bee273..ad7a6f475a44 100644
@@ -513,7 +513,13 @@ static void vhost_net_busy_poll(struct vhost_net *net,
 	struct socket *sock;
 	struct vhost_virtqueue *vq = poll_rx ? tvq : rvq;
 
-	mutex_lock_nested(&vq->mutex, poll_rx ? VHOST_NET_VQ_TX: VHOST_NET_VQ_RX);
+	/* Try to hold the vq mutex of the paired virtqueue. We can't
+	 * use mutex_lock() here since we could not guarantee a
+	 * consistenet lock ordering.
+	 */
+	if (!mutex_trylock(&vq->mutex))
+		return;
+
 	vhost_disable_notify(&net->dev, vq);
 	sock = rvq->private_data;

Signature · 1 line

2.17.1

Jason Wang

author

2018-12-13 02:53 UTC #4 lore raw

PATCH v3 2/3 vhost_net: switch to use mutex_trylock() in vhost_net_busy_poll()

In reply to Jason Wang

Fix this simply by switching to use mutex_trylock(), when fail just skip the busy polling. This can happen when device IOTLB is under updating which should be rare.

Fixes commit 78139c94dc8c ("net: vhost: lock the vqs one by one")

Cc Tonghao Zhang <[email protected]>

Acked-by Michael S. Tsirkin <[email protected]>

Signed-off-by Jason Wang <[email protected]>

---
 drivers/vhost/net.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/vhost/net.c +7 -1

index ab11b2bee273..ad7a6f475a44 100644
@@ -513,7 +513,13 @@ static void vhost_net_busy_poll(struct vhost_net *net,
 	struct socket *sock;
 	struct vhost_virtqueue *vq = poll_rx ? tvq : rvq;
 
-	mutex_lock_nested(&vq->mutex, poll_rx ? VHOST_NET_VQ_TX: VHOST_NET_VQ_RX);
+	/* Try to hold the vq mutex of the paired virtqueue. We can't
+	 * use mutex_lock() here since we could not guarantee a
+	 * consistenet lock ordering.
+	 */
+	if (!mutex_trylock(&vq->mutex))
+		return;
+
 	vhost_disable_notify(&net->dev, vq);
 	sock = rvq->private_data;

Signature · 1 line

2.17.1

Jason Wang

author

2018-12-13 02:53 UTC #5 lore raw

PATCH v3 3/3 Revert "net: vhost: lock the vqs one by one"

In reply to Jason Wang

This reverts commit 78139c94dc8c96a478e67dab3bee84dc6eccb5fd. We don't protect device IOTLB with vq mutex, which will lead e.g use after free for device IOTLB entries. And since we've switched to use mutex_trylock() in previous patch, it's safe to revert it without having deadlock.

Fixes commit 78139c94dc8c ("net: vhost: lock the vqs one by one")

Cc Tonghao Zhang <[email protected]>

Acked-by Michael S. Tsirkin <[email protected]>

Signed-off-by Jason Wang <[email protected]>

---
 drivers/vhost/vhost.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

drivers/vhost/vhost.c +17 -4

index 5915f240275a..55e5aa662ad5 100644
@@ -295,11 +295,8 @@ static void vhost_vq_meta_reset(struct vhost_dev *d)
 {
 	int i;
 
-	for (i = 0; i < d->nvqs; ++i) {
-		mutex_lock(&d->vqs[i]->mutex);
+	for (i = 0; i < d->nvqs; ++i)
 		__vhost_vq_meta_reset(d->vqs[i]);
-		mutex_unlock(&d->vqs[i]->mutex);
-	}
 }
 
 static void vhost_vq_reset(struct vhost_dev *dev,

@@ -895,6 +892,20 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq,
 #define vhost_get_used(vq, x, ptr) \
 	vhost_get_user(vq, x, ptr, VHOST_ADDR_USED)
 
+static void vhost_dev_lock_vqs(struct vhost_dev *d)
+{
+	int i = 0;
+	for (i = 0; i < d->nvqs; ++i)
+		mutex_lock_nested(&d->vqs[i]->mutex, i);
+}
+
+static void vhost_dev_unlock_vqs(struct vhost_dev *d)
+{
+	int i = 0;
+	for (i = 0; i < d->nvqs; ++i)
+		mutex_unlock(&d->vqs[i]->mutex);
+}
+
 static int vhost_new_umem_range(struct vhost_umem *umem,
 				u64 start, u64 size, u64 end,
 				u64 userspace_addr, int perm)

@@ -976,6 +987,7 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
 	int ret = 0;
 
 	mutex_lock(&dev->mutex);
+	vhost_dev_lock_vqs(dev);
 	switch (msg->type) {
 	case VHOST_IOTLB_UPDATE:
 		if (!dev->iotlb) {

@@ -1009,6 +1021,7 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
 		break;
 	}
 
+	vhost_dev_unlock_vqs(dev);
 	mutex_unlock(&dev->mutex);
 
 	return ret;

Signature · 1 line

2.17.1

Jason Wang

author

2018-12-13 02:53 UTC #6 lore raw

PATCH v3 3/3 Revert "net: vhost: lock the vqs one by one"

In reply to Jason Wang

Fixes commit 78139c94dc8c ("net: vhost: lock the vqs one by one")

Cc Tonghao Zhang <[email protected]>

Acked-by Michael S. Tsirkin <[email protected]>

Signed-off-by Jason Wang <[email protected]>

---
 drivers/vhost/vhost.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

drivers/vhost/vhost.c +17 -4

index 5915f240275a..55e5aa662ad5 100644
@@ -295,11 +295,8 @@ static void vhost_vq_meta_reset(struct vhost_dev *d)
 {
 	int i;
 
-	for (i = 0; i < d->nvqs; ++i) {
-		mutex_lock(&d->vqs[i]->mutex);
+	for (i = 0; i < d->nvqs; ++i)
 		__vhost_vq_meta_reset(d->vqs[i]);
-		mutex_unlock(&d->vqs[i]->mutex);
-	}
 }
 
 static void vhost_vq_reset(struct vhost_dev *dev,

@@ -895,6 +892,20 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq,
 #define vhost_get_used(vq, x, ptr) \
 	vhost_get_user(vq, x, ptr, VHOST_ADDR_USED)
 
+static void vhost_dev_lock_vqs(struct vhost_dev *d)
+{
+	int i = 0;
+	for (i = 0; i < d->nvqs; ++i)
+		mutex_lock_nested(&d->vqs[i]->mutex, i);
+}
+
+static void vhost_dev_unlock_vqs(struct vhost_dev *d)
+{
+	int i = 0;
+	for (i = 0; i < d->nvqs; ++i)
+		mutex_unlock(&d->vqs[i]->mutex);
+}
+
 static int vhost_new_umem_range(struct vhost_umem *umem,
 				u64 start, u64 size, u64 end,
 				u64 userspace_addr, int perm)

@@ -976,6 +987,7 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
 	int ret = 0;
 
 	mutex_lock(&dev->mutex);
+	vhost_dev_lock_vqs(dev);
 	switch (msg->type) {
 	case VHOST_IOTLB_UPDATE:
 		if (!dev->iotlb) {

@@ -1009,6 +1021,7 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
 		break;
 	}
 
+	vhost_dev_unlock_vqs(dev);
 	mutex_unlock(&dev->mutex);
 
 	return ret;

Signature · 1 line

2.17.1

David Miller

maintainer

2018-12-13 05:57 UTC #7 lore raw

In reply to Jason Wang

From: Jason Wang <[email protected]>
Date: Thu, 13 Dec 2018 10:53:36 +0800

Quoted 9 lines

This series tries to fix various issues of vhost:
- Patch 1 adds a missing write barrier between used idx updating and logging.
- Patch 2-3 brings back the protection of device IOTLB through vq mutex, this fixes possible use after free in device IOTLB entries.
Please consider them for -stable.

Quoted 8 lines

Thanks
Changes from V2:
- drop dirty page fix and make it for net-next
Changes from V1:
- silent compiler warning for 32bit.
- use mutex_trylock() on slowpath instead of mutex_lock() even on fast path.

Series applied and queued up for -stable, thanks Jason.

David Miller

maintainer

2018-12-13 05:57 UTC #8 lore raw

In reply to Jason Wang

From: Jason Wang <[email protected]>
Date: Thu, 13 Dec 2018 10:53:36 +0800

Quoted 9 lines

This series tries to fix various issues of vhost:
- Patch 1 adds a missing write barrier between used idx updating and logging.
- Patch 2-3 brings back the protection of device IOTLB through vq mutex, this fixes possible use after free in device IOTLB entries.
Please consider them for -stable.

Quoted 8 lines

Thanks
Changes from V2:
- drop dirty page fix and make it for net-next
Changes from V1:
- silent compiler warning for 32bit.
- use mutex_trylock() on slowpath instead of mutex_lock() even on fast path.

Series applied and queued up for -stable, thanks Jason.

Michael S. Tsirkin

maintainer Acked-by on the v3 series

2018-12-13 14:32 UTC #9 lore raw

In reply to Jason Wang

Quoted 10 lines · Jason Wang

Hi:
This series tries to fix various issues of vhost:
- Patch 1 adds a missing write barrier between used idx updating and logging.
- Patch 2-3 brings back the protection of device IOTLB through vq mutex, this fixes possible use after free in device IOTLB entries.
Please consider them for -stable.

Acked-by Michael S. Tsirkin <[email protected]>

Quoted 20 lines

Thanks
Changes from V2:
- drop dirty page fix and make it for net-next
Changes from V1:
- silent compiler warning for 32bit.
- use mutex_trylock() on slowpath instead of mutex_lock() even on fast path.
Jason Wang (3):
vhost: make sure used idx is seen before log in vhost_add_used_n()
vhost_net: switch to use mutex_trylock() in vhost_net_busy_poll() Revert "net: vhost: lock the vqs one by one"
 drivers/vhost/net.c   |  8 +++++++-
 drivers/vhost/vhost.c | 23 +++++++++++++++++++----
 2 files changed, 26 insertions(+), 5 deletions(-)
Signature · 1 line
2.17.1

Michael S. Tsirkin

maintainer Acked-by on the v3 series

2018-12-13 14:32 UTC #10 lore raw

In reply to Jason Wang

Quoted 10 lines · Jason Wang

Hi:
This series tries to fix various issues of vhost:
- Patch 1 adds a missing write barrier between used idx updating and logging.
- Patch 2-3 brings back the protection of device IOTLB through vq mutex, this fixes possible use after free in device IOTLB entries.
Please consider them for -stable.

Acked-by Michael S. Tsirkin <[email protected]>

Quoted 20 lines

Thanks
Changes from V2:
- drop dirty page fix and make it for net-next
Changes from V1:
- silent compiler warning for 32bit.
- use mutex_trylock() on slowpath instead of mutex_lock() even on fast path.
Jason Wang (3):
vhost: make sure used idx is seen before log in vhost_add_used_n()
vhost_net: switch to use mutex_trylock() in vhost_net_busy_poll() Revert "net: vhost: lock the vqs one by one"
 drivers/vhost/net.c   |  8 +++++++-
 drivers/vhost/vhost.c | 23 +++++++++++++++++++----
 2 files changed, 26 insertions(+), 5 deletions(-)
Signature · 1 line
2.17.1