Linux Virtualization - Jun 2013 - [PATCH net 2/2] vhost: fix ubuf_info cleanup

Two patches fixing the fallout from the vhost cleanup in 3.10.
Thanks to Tommi Rantala who reported the issue.

Tommi, could you please confirm this fixes the crashes for you?

Michael S. Tsirkin (2):
  vhost: check owner before we overwrite ubuf_info
  vhost: fix ubuf_info cleanup

 drivers/vhost/net.c   | 26 +++++++++++---------------
 drivers/vhost/vhost.c |  8 +++++++-
 drivers/vhost/vhost.h |  1 +
 3 files changed, 19 insertions(+), 16 deletions(-)

-- 
MST

If device has an owner, we shouldn't touch ubuf_info
since it might be in use.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 drivers/vhost/net.c   | 4 ++++
 drivers/vhost/vhost.c | 8 +++++++-
 drivers/vhost/vhost.h | 1 +
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 2b51e23..6b00f64 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -1053,6 +1053,10 @@ static long vhost_net_set_owner(struct vhost_net *n)
 	int r;
 
 	mutex_lock(&n->dev.mutex);
+	if (vhost_dev_has_owner(&n->dev)) {
+		r = -EBUSY;
+		goto out;
+	}
 	r = vhost_net_set_ubuf_info(n);
 	if (r)
 		goto out;
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index beee7f5..60aa5ad 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -344,13 +344,19 @@ static int vhost_attach_cgroups(struct vhost_dev *dev)
 }
 
 /* Caller should have device mutex */
+bool vhost_dev_has_owner(struct vhost_dev *dev)
+{
+	return dev->mm;
+}
+
+/* Caller should have device mutex */
 long vhost_dev_set_owner(struct vhost_dev *dev)
 {
 	struct task_struct *worker;
 	int err;
 
 	/* Is there an owner already? */
-	if (dev->mm) {
+	if (vhost_dev_has_owner(dev)) {
 		err = -EBUSY;
 		goto err_mm;
 	}
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index a7ad635..64adcf9 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -133,6 +133,7 @@ struct vhost_dev {
 
 long vhost_dev_init(struct vhost_dev *, struct vhost_virtqueue **vqs, int
nvqs);
 long vhost_dev_set_owner(struct vhost_dev *dev);
+bool vhost_dev_has_owner(struct vhost_dev *dev);
 long vhost_dev_check_owner(struct vhost_dev *);
 struct vhost_memory *vhost_dev_reset_owner_prepare(void);
 void vhost_dev_reset_owner(struct vhost_dev *, struct vhost_memory *);
-- 
MST

vhost_net_clear_ubuf_info didn't clear ubuf_info
after kfree, this could trigger double free.
Fix this and simplify this code to make it more robust: make sure
ubuf info is always freed through vhost_net_clear_ubuf_info.

Reported-by: Tommi Rantala <tt.rantala at gmail.com>
Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 drivers/vhost/net.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 6b00f64..7fc47f7 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -155,14 +155,11 @@ static void vhost_net_ubuf_put_and_wait(struct
vhost_net_ubuf_ref *ubufs)
 
 static void vhost_net_clear_ubuf_info(struct vhost_net *n)
 {
-
-	bool zcopy;
 	int i;
 
-	for (i = 0; i < n->dev.nvqs; ++i) {
-		zcopy = vhost_net_zcopy_mask & (0x1 << i);
-		if (zcopy)
-			kfree(n->vqs[i].ubuf_info);
+	for (i = 0; i < VHOST_NET_VQ_MAX; ++i) {
+		kfree(n->vqs[i].ubuf_info);
+		n->vqs[i].ubuf_info = NULL;
 	}
 }
 
@@ -171,7 +168,7 @@ int vhost_net_set_ubuf_info(struct vhost_net *n)
 	bool zcopy;
 	int i;
 
-	for (i = 0; i < n->dev.nvqs; ++i) {
+	for (i = 0; i < VHOST_NET_VQ_MAX; ++i) {
 		zcopy = vhost_net_zcopy_mask & (0x1 << i);
 		if (!zcopy)
 			continue;
@@ -183,12 +180,7 @@ int vhost_net_set_ubuf_info(struct vhost_net *n)
 	return 0;
 
 err:
-	while (i--) {
-		zcopy = vhost_net_zcopy_mask & (0x1 << i);
-		if (!zcopy)
-			continue;
-		kfree(n->vqs[i].ubuf_info);
-	}
+	vhost_net_clear_ubuf_info(n);
 	return -ENOMEM;
 }
 
@@ -196,12 +188,12 @@ void vhost_net_vq_reset(struct vhost_net *n)
 {
 	int i;
 
+	vhost_net_clear_ubuf_info(n);
+
 	for (i = 0; i < VHOST_NET_VQ_MAX; i++) {
 		n->vqs[i].done_idx = 0;
 		n->vqs[i].upend_idx = 0;
 		n->vqs[i].ubufs = NULL;
-		kfree(n->vqs[i].ubuf_info);
-		n->vqs[i].ubuf_info = NULL;
 		n->vqs[i].vhost_hlen = 0;
 		n->vqs[i].sock_hlen = 0;
 	}
-- 
MST

2013/6/6 Michael S. Tsirkin <mst at redhat.com>:
> Two patches fixing the fallout from the vhost cleanup in 3.10.
> Thanks to Tommi Rantala who reported the issue.
>
> Tommi, could you please confirm this fixes the crashes for you?
Confirmed! With the two patches applied, I can no longer reproduce the
crash with trinity.

Thanks!

Tommi
> Michael S. Tsirkin (2):
>   vhost: check owner before we overwrite ubuf_info
>   vhost: fix ubuf_info cleanup
>
>  drivers/vhost/net.c   | 26 +++++++++++---------------
>  drivers/vhost/vhost.c |  8 +++++++-
>  drivers/vhost/vhost.h |  1 +
>  3 files changed, 19 insertions(+), 16 deletions(-)
>
> --
> MST
>

From: "Michael S. Tsirkin" <mst at redhat.com>
Date: Thu, 6 Jun 2013 15:20:39 +0300
> If device has an owner, we shouldn't touch ubuf_info
> since it might be in use.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
Applied.

From: "Michael S. Tsirkin" <mst at redhat.com>
Date: Thu, 6 Jun 2013 15:20:46 +0300
> vhost_net_clear_ubuf_info didn't clear ubuf_info
> after kfree, this could trigger double free.
> Fix this and simplify this code to make it more robust: make sure
> ubuf info is always freed through vhost_net_clear_ubuf_info.
> 
> Reported-by: Tommi Rantala <tt.rantala at gmail.com>
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
Applied.