[PATCH 0/3] vhost: logging fixes

The following patches on top of net-next fix issues related to write
logging in vhost. This fixes all known to me logging issues, migration
now works for me while under stress in both TX and RX directions.
Rusty's going on vacation, I am guessing he won't have time to review
this: Gleb, Juan, Herbert, could one of you review this patchset please?

There's also the send queue full issue reported by
Sridhar Samudrala which I'm testing various fixes for,
that patch is contained to vhost/net though,
so there's no conflict, patch will be posted separately.


Michael S. Tsirkin (3):
  vhost: logging thinko fix
  vhost: initialize log eventfd context pointer
  vhost: fix get_user_pages_fast error handling

 drivers/vhost/vhost.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

vhost was dong some complex math to get
offset to log at, and got it wrong by a couple of bytes,
while in fact it's simple: get address where we write,
subtract start of buffer, add log base.

Do it this way.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/vhost/vhost.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 6eb1525..c767279 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c

@@ -1004,10 +1004,12 @@ int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int len)
 	if (unlikely(vq->log_used)) {
 		/* Make sure data is seen before log. */
 		smp_wmb();
-		log_write(vq->log_base, vq->log_addr + sizeof *vq->used->ring *
-			  (vq->last_used_idx % vq->num),
-			  sizeof *vq->used->ring);
-		log_write(vq->log_base, vq->log_addr, sizeof *vq->used->ring);
+		log_write(vq->log_base,
+			  vq->log_addr + ((void *)used - (void *)vq->used),
+			  sizeof *used);
+		log_write(vq->log_base,
+			  vq->log_addr + offsetof(struct vring_used, idx),
+			  sizeof vq->used->idx);
 		if (vq->log_ctx)
 			eventfd_signal(vq->log_ctx, 1);
 	}

-- 
1.7.0.18.g0d53a5

vq log eventfd context pointer needs to be initialized, otherwise
operation may fail or oops if log is enabled but log eventfd not set by
userspace.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/vhost/vhost.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index c767279..d4f8fdf 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c

@@ -121,6 +121,7 @@ static void vhost_vq_reset(struct vhost_dev *dev,
 	vq->kick = NULL;
 	vq->call_ctx = NULL;
 	vq->call = NULL;
+	vq->log_ctx = NULL;
 }
 
 long vhost_dev_init(struct vhost_dev *dev,

-- 
1.7.0.18.g0d53a5

get_user_pages_fast returns number of pages on success, negative value
on failure, but never 0. Fix vhost code to match this logic.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/vhost/vhost.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index d4f8fdf..d003504 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c

@@ -646,8 +646,9 @@ static int set_bit_to_user(int nr, void __user *addr)
 	int bit = nr + (log % PAGE_SIZE) * 8;
 	int r;
 	r = get_user_pages_fast(log, 1, 1, &page);
-	if (r)
+	if (r < 0)
 		return r;
+	BUG_ON(r != 1);
 	base = kmap_atomic(page, KM_USER0);
 	set_bit(bit, base);
 	kunmap_atomic(base, KM_USER0);

-- 
1.7.0.18.g0d53a5

On Tue, Feb 23, 2010 at 06:57:58PM +0200, Michael S. Tsirkin wrote:

Can't this be easily triggered from user space?

 	base = kmap_atomic(page, KM_USER0);
 	set_bit(bit, base);
 	kunmap_atomic(base, KM_USER0);
-- 
1.7.0.18.g0d53a5

--
			Gleb.

On Tue, Feb 23, 2010 at 07:34:34PM +0200, Gleb Natapov wrote:

On Tue, Feb 23, 2010 at 06:57:58PM +0200, Michael S. Tsirkin wrote:

quoted

get_user_pages_fast returns number of pages on success, negative value
on failure, but never 0. Fix vhost code to match this logic.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/vhost/vhost.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index d4f8fdf..d003504 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c

@@ -646,8 +646,9 @@ static int set_bit_to_user(int nr, void __user *addr)
 	int bit = nr + (log % PAGE_SIZE) * 8;
 	int r;
 	r = get_user_pages_fast(log, 1, 1, &page);
-	if (r)
+	if (r < 0)
 		return r;
+	BUG_ON(r != 1);

Can't this be easily triggered from user space?

I think no. get_user_pages_fast always returns number of pages
pinned (in this case always 1) or an error (< 0).
Anything else is a kernel bug.

quoted

 	base = kmap_atomic(page, KM_USER0);
 	set_bit(bit, base);
 	kunmap_atomic(base, KM_USER0);
-- 
1.7.0.18.g0d53a5

--
			Gleb.

On Tue, Feb 23, 2010 at 07:32:58PM +0200, Michael S. Tsirkin wrote:

On Tue, Feb 23, 2010 at 07:34:34PM +0200, Gleb Natapov wrote:

quoted

On Tue, Feb 23, 2010 at 06:57:58PM +0200, Michael S. Tsirkin wrote:

quoted

get_user_pages_fast returns number of pages on success, negative value
on failure, but never 0. Fix vhost code to match this logic.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/vhost/vhost.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index d4f8fdf..d003504 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c

@@ -646,8 +646,9 @@ static int set_bit_to_user(int nr, void __user *addr)
 	int bit = nr + (log % PAGE_SIZE) * 8;
 	int r;
 	r = get_user_pages_fast(log, 1, 1, &page);
-	if (r)
+	if (r < 0)
 		return r;
+	BUG_ON(r != 1);

Can't this be easily triggered from user space?

I think no. get_user_pages_fast always returns number of pages
pinned (in this case always 1) or an error (< 0).
Anything else is a kernel bug.

But what if page is unmapped from userspace?

quoted

quoted

 	base = kmap_atomic(page, KM_USER0);
 	set_bit(bit, base);
 	kunmap_atomic(base, KM_USER0);
-- 
1.7.0.18.g0d53a5

--
			Gleb.

--
			Gleb.

On Tue, Feb 23, 2010 at 07:39:52PM +0200, Gleb Natapov wrote:

On Tue, Feb 23, 2010 at 07:32:58PM +0200, Michael S. Tsirkin wrote:

quoted

On Tue, Feb 23, 2010 at 07:34:34PM +0200, Gleb Natapov wrote:

quoted

On Tue, Feb 23, 2010 at 06:57:58PM +0200, Michael S. Tsirkin wrote:

quoted

get_user_pages_fast returns number of pages on success, negative value
on failure, but never 0. Fix vhost code to match this logic.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/vhost/vhost.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index d4f8fdf..d003504 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c

@@ -646,8 +646,9 @@ static int set_bit_to_user(int nr, void __user *addr)
 	int bit = nr + (log % PAGE_SIZE) * 8;
 	int r;
 	r = get_user_pages_fast(log, 1, 1, &page);
-	if (r)
+	if (r < 0)
 		return r;
+	BUG_ON(r != 1);

Can't this be easily triggered from user space?

I think no. get_user_pages_fast always returns number of pages
pinned (in this case always 1) or an error (< 0).
Anything else is a kernel bug.

But what if page is unmapped from userspace?

Then we get -EFAULT

quoted

quoted

quoted

 	base = kmap_atomic(page, KM_USER0);
 	set_bit(bit, base);
 	kunmap_atomic(base, KM_USER0);
-- 
1.7.0.18.g0d53a5

--
			Gleb.

--
			Gleb.

On Tue, Feb 23, 2010 at 07:39:08PM +0200, Michael S. Tsirkin wrote:

On Tue, Feb 23, 2010 at 07:39:52PM +0200, Gleb Natapov wrote:

quoted

On Tue, Feb 23, 2010 at 07:32:58PM +0200, Michael S. Tsirkin wrote:

quoted

On Tue, Feb 23, 2010 at 07:34:34PM +0200, Gleb Natapov wrote:

quoted

On Tue, Feb 23, 2010 at 06:57:58PM +0200, Michael S. Tsirkin wrote:

quoted

get_user_pages_fast returns number of pages on success, negative value
on failure, but never 0. Fix vhost code to match this logic.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/vhost/vhost.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index d4f8fdf..d003504 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c

@@ -646,8 +646,9 @@ static int set_bit_to_user(int nr, void __user *addr)
 	int bit = nr + (log % PAGE_SIZE) * 8;
 	int r;
 	r = get_user_pages_fast(log, 1, 1, &page);
-	if (r)
+	if (r < 0)
 		return r;
+	BUG_ON(r != 1);

Can't this be easily triggered from user space?

I think no. get_user_pages_fast always returns number of pages
pinned (in this case always 1) or an error (< 0).
Anything else is a kernel bug.

But what if page is unmapped from userspace?

Then we get -EFAULT

Ah correct.

quoted

quoted

quoted

quoted

 	base = kmap_atomic(page, KM_USER0);
 	set_bit(bit, base);
 	kunmap_atomic(base, KM_USER0);
-- 
1.7.0.18.g0d53a5

--
			Gleb.

--
			Gleb.

--
			Gleb.

Just for the record I'm generally not interested in vhost
patches.

If it's a specific network one that will be merged via
the networking tree, yes please CC: me.

But if it's a bunch of changes to vhost.c and other pieces
of infrastructure, feel free to leave me out of it.  It just
clutters my already overflowing inbox.

Thanks.

On Tue, Feb 23, 2010 at 02:42:35PM -0800, David Miller wrote:

Just for the record I'm generally not interested in vhost
patches.

If it's a specific network one that will be merged via
the networking tree, yes please CC: me.

But if it's a bunch of changes to vhost.c and other pieces
of infrastructure, feel free to leave me out of it.  It just
clutters my already overflowing inbox.

Thanks.

Dave, so while Rusty's on vacation, what's the best way to get vhost
infrastructure fixes in? Are you ok with getting pull requests and
merging them into net-next?  That should keep the clutter in your inbox
to the minimum.

Of course network changes would still go the usual way.

-- 
MST

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Wed, 24 Feb 2010 07:37:37 +0200

Dave, so while Rusty's on vacation, what's the best way to get vhost
infrastructure fixes in? Are you ok with getting pull requests and
merging them into net-next?  That should keep the clutter in your inbox
to the minimum.

Of course network changes would still go the usual way.

Well, who is providing oversight of vhost work while he's
gone?  Has he, implicitly or explicitly, appointed a maintainer
while he's away?

On Tue, Feb 23, 2010 at 11:04:28PM -0800, David Miller wrote:

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Wed, 24 Feb 2010 07:37:37 +0200

quoted

Dave, so while Rusty's on vacation, what's the best way to get vhost
infrastructure fixes in? Are you ok with getting pull requests and
merging them into net-next?  That should keep the clutter in your inbox
to the minimum.

Of course network changes would still go the usual way.

Well, who is providing oversight of vhost work while he's
gone?

My plan was to get peer review of the patches before merging.
So far Juan Quintela and Gleb Natapov gave feedback.

Has he, implicitly or explicitly, appointed a maintainer
while he's away?

Implicitly, I guess. He said "if there's an issue Michael Tsirkin is the
best person to resolve it", this was wrt merging his virtio&lguest tree.
He didn't mention vhost, I wrote all of vhost though, there shouldn't be
an issue with that.

-- 
MST

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Wed, 24 Feb 2010 09:34:25 +0200

Implicitly, I guess. He said "if there's an issue Michael Tsirkin is the
best person to resolve it", this was wrt merging his virtio&lguest tree.
He didn't mention vhost, I wrote all of vhost though, there shouldn't be
an issue with that.

That's good enough for me.

Feel free to setup a tree for me to pull from.