2013-06-06 12:28:50

by Michael S. Tsirkin

Subject: [PATCH net 0/2] vhost fixes for 3.10

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

2013-06-06 12:20:19

by Michael S. Tsirkin

Subject: [PATCH net 1/2] vhost: check owner before we overwrite ubuf_info

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

Signed-off-by: Michael S. Tsirkin <[email protected]>
---
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

2013-06-06 12:20:25

by Michael S. Tsirkin

Subject: [PATCH net 2/2] vhost: fix ubuf_info cleanup

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 <[email protected]>
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
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-06-06 18:50:43

by Tommi Rantala

Subject: Re: [PATCH net 0/2] vhost fixes for 3.10

2013/6/6 Michael S. Tsirkin <[email protected]>:
> 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
>

2013-06-11 09:46:55

by David Miller

Subject: Re: [PATCH net 1/2] vhost: check owner before we overwrite ubuf_info

From: "Michael S. Tsirkin" <[email protected]>
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 <[email protected]>

Applied.

2013-06-11 09:47:01

by David Miller

Subject: Re: [PATCH net 2/2] vhost: fix ubuf_info cleanup

From: "Michael S. Tsirkin" <[email protected]>
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 <[email protected]>
> Signed-off-by: Michael S. Tsirkin <[email protected]>

Applied.