* [PATCH net V3 1/3] vhost: make sure used idx is seen before log in vhost_add_used_n()
2018-12-13 2:53 [PATCH net V3 0/3] Fix various issue of vhost Jason Wang
@ 2018-12-13 2:53 ` Jason Wang
2018-12-13 2:53 ` [PATCH net V3 2/3] vhost_net: switch to use mutex_trylock() in vhost_net_busy_poll() Jason Wang
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Jason Wang @ 2018-12-13 2:53 UTC (permalink / raw)
To: mst, jasowang, kvm, virtualization, netdev, linux-kernel
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 <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/vhost.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 6b98d8e3a5bf..5915f240275a 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -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),
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH net V3 2/3] vhost_net: switch to use mutex_trylock() in vhost_net_busy_poll()
2018-12-13 2:53 [PATCH net V3 0/3] Fix various issue of vhost Jason Wang
2018-12-13 2:53 ` [PATCH net V3 1/3] vhost: make sure used idx is seen before log in vhost_add_used_n() Jason Wang
@ 2018-12-13 2:53 ` Jason Wang
2018-12-13 2:53 ` [PATCH net V3 3/3] Revert "net: vhost: lock the vqs one by one" Jason Wang
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Jason Wang @ 2018-12-13 2:53 UTC (permalink / raw)
To: mst, jasowang, kvm, virtualization, netdev, linux-kernel; +Cc: Tonghao Zhang
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 <xiangxia.m.yue@gmail.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/net.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index ab11b2bee273..ad7a6f475a44 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -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;
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH net V3 3/3] Revert "net: vhost: lock the vqs one by one"
2018-12-13 2:53 [PATCH net V3 0/3] Fix various issue of vhost Jason Wang
2018-12-13 2:53 ` [PATCH net V3 1/3] vhost: make sure used idx is seen before log in vhost_add_used_n() Jason Wang
2018-12-13 2:53 ` [PATCH net V3 2/3] vhost_net: switch to use mutex_trylock() in vhost_net_busy_poll() Jason Wang
@ 2018-12-13 2:53 ` Jason Wang
2018-12-13 5:57 ` [PATCH net V3 0/3] Fix various issue of vhost David Miller
2018-12-13 14:32 ` Michael S. Tsirkin
4 siblings, 0 replies; 6+ messages in thread
From: Jason Wang @ 2018-12-13 2:53 UTC (permalink / raw)
To: mst, jasowang, kvm, virtualization, netdev, linux-kernel; +Cc: Tonghao Zhang
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 <xiangxia.m.yue@gmail.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/vhost.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 5915f240275a..55e5aa662ad5 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -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;
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net V3 0/3] Fix various issue of vhost
2018-12-13 2:53 [PATCH net V3 0/3] Fix various issue of vhost Jason Wang
` (2 preceding siblings ...)
2018-12-13 2:53 ` [PATCH net V3 3/3] Revert "net: vhost: lock the vqs one by one" Jason Wang
@ 2018-12-13 5:57 ` David Miller
2018-12-13 14:32 ` Michael S. Tsirkin
4 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2018-12-13 5:57 UTC (permalink / raw)
To: jasowang; +Cc: mst, kvm, virtualization, netdev, linux-kernel
From: Jason Wang <jasowang@redhat.com>
Date: Thu, 13 Dec 2018 10:53:36 +0800
> 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.
Series applied and queued up for -stable, thanks Jason.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net V3 0/3] Fix various issue of vhost
2018-12-13 2:53 [PATCH net V3 0/3] Fix various issue of vhost Jason Wang
` (3 preceding siblings ...)
2018-12-13 5:57 ` [PATCH net V3 0/3] Fix various issue of vhost David Miller
@ 2018-12-13 14:32 ` Michael S. Tsirkin
4 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2018-12-13 14:32 UTC (permalink / raw)
To: Jason Wang; +Cc: kvm, virtualization, netdev, linux-kernel
On Thu, Dec 13, 2018 at 10:53:36AM +0800, Jason Wang wrote:
> 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 <mst@redhat.com>
> 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(-)
>
> --
> 2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread