* [PATCH 0/3] vhost: logging fixes
@ 2010-02-23 16:57 ` Michael S. Tsirkin
0 siblings, 0 replies; 23+ messages in thread
From: Michael S. Tsirkin @ 2010-02-23 16:57 UTC (permalink / raw)
To: Rusty Russell, kvm, virtualization, netdev, linux-kernel,
David Miller, gl
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(-)
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 1/3] vhost: logging math fix
2010-02-23 16:57 ` Michael S. Tsirkin
@ 2010-02-23 16:57 ` Michael S. Tsirkin
-1 siblings, 0 replies; 23+ messages in thread
From: Michael S. Tsirkin @ 2010-02-23 16:57 UTC (permalink / raw)
To: Rusty Russell, kvm, virtualization, netdev, linux-kernel,
David Miller, markmc, gleb, herbert.xu, quintela, dlaor, avi
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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 1/3] vhost: logging math fix
@ 2010-02-23 16:57 ` Michael S. Tsirkin
0 siblings, 0 replies; 23+ messages in thread
From: Michael S. Tsirkin @ 2010-02-23 16:57 UTC (permalink / raw)
To: Rusty Russell, kvm, virtualization, netdev, linux-kernel,
David Miller, ma
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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 1/3] vhost: logging math fix
2010-02-23 16:57 ` Michael S. Tsirkin
(?)
@ 2010-02-23 19:26 ` Juan Quintela
-1 siblings, 0 replies; 23+ messages in thread
From: Juan Quintela @ 2010-02-23 19:26 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rusty Russell, kvm, virtualization, netdev, linux-kernel,
David Miller, markmc, gleb, herbert.xu, dlaor, avi
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> 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>
Reviewed-by: Juan Quintela <quintela@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. */
We explain what smp_wmb() does.
> 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);
Once here, can we add a comment explaining _what_ are we trying to write
to the log? michael explains that t is the used element and the index,
but nothing states that.
> if (vq->log_ctx)
> eventfd_signal(vq->log_ctx, 1);
> }
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/3] vhost: initialize log eventfd context pointer
2010-02-23 16:57 ` Michael S. Tsirkin
@ 2010-02-23 16:57 ` Michael S. Tsirkin
-1 siblings, 0 replies; 23+ messages in thread
From: Michael S. Tsirkin @ 2010-02-23 16:57 UTC (permalink / raw)
To: Rusty Russell, kvm, virtualization, netdev, linux-kernel,
David Miller, markmc, gleb, herbert.xu, quintela, dlaor, avi
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 2/3] vhost: initialize log eventfd context pointer
@ 2010-02-23 16:57 ` Michael S. Tsirkin
0 siblings, 0 replies; 23+ messages in thread
From: Michael S. Tsirkin @ 2010-02-23 16:57 UTC (permalink / raw)
To: Rusty Russell, kvm, virtualization, netdev, linux-kernel,
David Miller, ma
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 2/3] vhost: initialize log eventfd context pointer
2010-02-23 16:57 ` Michael S. Tsirkin
(?)
@ 2010-02-23 19:31 ` Juan Quintela
-1 siblings, 0 replies; 23+ messages in thread
From: Juan Quintela @ 2010-02-23 19:31 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rusty Russell, kvm, virtualization, netdev, linux-kernel,
David Miller, markmc, gleb, herbert.xu, dlaor, avi
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> 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>
Reviewed-by: Juan Quintela <quintela@redhat.com>
When log_ctx for device is created, it is copied to the vq. This reset
was missing.
> ---
> 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,
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 3/3] vhost: fix get_user_pages_fast error handling
2010-02-23 16:57 ` Michael S. Tsirkin
@ 2010-02-23 16:57 ` Michael S. Tsirkin
-1 siblings, 0 replies; 23+ messages in thread
From: Michael S. Tsirkin @ 2010-02-23 16:57 UTC (permalink / raw)
To: Rusty Russell, kvm, virtualization, netdev, linux-kernel,
David Miller, markmc, gleb, herbert.xu, quintela, dlaor, avi
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 3/3] vhost: fix get_user_pages_fast error handling
@ 2010-02-23 16:57 ` Michael S. Tsirkin
0 siblings, 0 replies; 23+ messages in thread
From: Michael S. Tsirkin @ 2010-02-23 16:57 UTC (permalink / raw)
To: Rusty Russell, kvm, virtualization, netdev, linux-kernel,
David Miller, ma
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling
2010-02-23 16:57 ` Michael S. Tsirkin
(?)
@ 2010-02-23 17:34 ` Gleb Natapov
2010-02-23 17:32 ` Michael S. Tsirkin
-1 siblings, 1 reply; 23+ messages in thread
From: Gleb Natapov @ 2010-02-23 17:34 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rusty Russell, kvm, virtualization, netdev, linux-kernel,
David Miller, markmc, herbert.xu, quintela, dlaor, avi
On Tue, Feb 23, 2010 at 06:57:58PM +0200, Michael S. Tsirkin wrote:
> 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?
> base = kmap_atomic(page, KM_USER0);
> set_bit(bit, base);
> kunmap_atomic(base, KM_USER0);
> --
> 1.7.0.18.g0d53a5
--
Gleb.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling
2010-02-23 17:34 ` Gleb Natapov
@ 2010-02-23 17:32 ` Michael S. Tsirkin
2010-02-23 17:39 ` Gleb Natapov
0 siblings, 1 reply; 23+ messages in thread
From: Michael S. Tsirkin @ 2010-02-23 17:32 UTC (permalink / raw)
To: Gleb Natapov
Cc: Rusty Russell, kvm, virtualization, netdev, linux-kernel,
David Miller, markmc, herbert.xu, quintela, dlaor, avi
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:
> > 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.
> > base = kmap_atomic(page, KM_USER0);
> > set_bit(bit, base);
> > kunmap_atomic(base, KM_USER0);
> > --
> > 1.7.0.18.g0d53a5
>
> --
> Gleb.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling
2010-02-23 17:32 ` Michael S. Tsirkin
@ 2010-02-23 17:39 ` Gleb Natapov
2010-02-23 17:39 ` Michael S. Tsirkin
2010-02-23 22:42 ` David Miller
0 siblings, 2 replies; 23+ messages in thread
From: Gleb Natapov @ 2010-02-23 17:39 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rusty Russell, kvm, virtualization, netdev, linux-kernel,
David Miller, markmc, herbert.xu, quintela, dlaor, avi
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:
> > On Tue, Feb 23, 2010 at 06:57:58PM +0200, Michael S. Tsirkin wrote:
> > > 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?
> > > base = kmap_atomic(page, KM_USER0);
> > > set_bit(bit, base);
> > > kunmap_atomic(base, KM_USER0);
> > > --
> > > 1.7.0.18.g0d53a5
> >
> > --
> > Gleb.
--
Gleb.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling
2010-02-23 17:39 ` Gleb Natapov
@ 2010-02-23 17:39 ` Michael S. Tsirkin
2010-02-23 17:43 ` Gleb Natapov
2010-02-23 22:42 ` David Miller
1 sibling, 1 reply; 23+ messages in thread
From: Michael S. Tsirkin @ 2010-02-23 17:39 UTC (permalink / raw)
To: Gleb Natapov
Cc: Rusty Russell, kvm, virtualization, netdev, linux-kernel,
David Miller, markmc, herbert.xu, quintela, dlaor, avi
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:
> > 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:
> > > > 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
> > > > base = kmap_atomic(page, KM_USER0);
> > > > set_bit(bit, base);
> > > > kunmap_atomic(base, KM_USER0);
> > > > --
> > > > 1.7.0.18.g0d53a5
> > >
> > > --
> > > Gleb.
>
> --
> Gleb.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling
2010-02-23 17:39 ` Michael S. Tsirkin
@ 2010-02-23 17:43 ` Gleb Natapov
0 siblings, 0 replies; 23+ messages in thread
From: Gleb Natapov @ 2010-02-23 17:43 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rusty Russell, kvm, virtualization, netdev, linux-kernel,
David Miller, markmc, herbert.xu, quintela, dlaor, avi
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:
> > 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:
> > > > On Tue, Feb 23, 2010 at 06:57:58PM +0200, Michael S. Tsirkin wrote:
> > > > > 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.
> > > > > base = kmap_atomic(page, KM_USER0);
> > > > > set_bit(bit, base);
> > > > > kunmap_atomic(base, KM_USER0);
> > > > > --
> > > > > 1.7.0.18.g0d53a5
> > > >
> > > > --
> > > > Gleb.
> >
> > --
> > Gleb.
--
Gleb.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling
2010-02-23 17:39 ` Gleb Natapov
2010-02-23 17:39 ` Michael S. Tsirkin
@ 2010-02-23 22:42 ` David Miller
2010-02-24 5:37 ` Michael S. Tsirkin
1 sibling, 1 reply; 23+ messages in thread
From: David Miller @ 2010-02-23 22:42 UTC (permalink / raw)
To: gleb
Cc: mst, rusty, kvm, virtualization, netdev, linux-kernel, markmc,
herbert.xu, quintela, dlaor, avi
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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling
2010-02-23 22:42 ` David Miller
@ 2010-02-24 5:37 ` Michael S. Tsirkin
2010-02-24 7:04 ` David Miller
0 siblings, 1 reply; 23+ messages in thread
From: Michael S. Tsirkin @ 2010-02-24 5:37 UTC (permalink / raw)
To: David Miller
Cc: gleb, rusty, kvm, virtualization, netdev, linux-kernel, markmc,
herbert.xu, quintela, dlaor, avi
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling
2010-02-24 5:37 ` Michael S. Tsirkin
@ 2010-02-24 7:04 ` David Miller
2010-02-24 7:34 ` Michael S. Tsirkin
0 siblings, 1 reply; 23+ messages in thread
From: David Miller @ 2010-02-24 7:04 UTC (permalink / raw)
To: mst
Cc: gleb, rusty, kvm, virtualization, netdev, linux-kernel, markmc,
herbert.xu, quintela, dlaor, avi
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?
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling
2010-02-24 7:04 ` David Miller
@ 2010-02-24 7:34 ` Michael S. Tsirkin
0 siblings, 0 replies; 23+ messages in thread
From: Michael S. Tsirkin @ 2010-02-24 7:34 UTC (permalink / raw)
To: David Miller
Cc: gleb, rusty, kvm, virtualization, netdev, linux-kernel, markmc,
herbert.xu, quintela, dlaor, avi
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
>
> > 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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling
@ 2010-02-24 7:34 ` Michael S. Tsirkin
0 siblings, 0 replies; 23+ messages in thread
From: Michael S. Tsirkin @ 2010-02-24 7:34 UTC (permalink / raw)
To: David Miller
Cc: markmc, kvm, quintela, netdev, linux-kernel, virtualization, avi,
herbert.xu
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
>
> > 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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling
2010-02-24 7:34 ` Michael S. Tsirkin
(?)
@ 2010-02-24 7:41 ` David Miller
-1 siblings, 0 replies; 23+ messages in thread
From: David Miller @ 2010-02-24 7:41 UTC (permalink / raw)
To: mst
Cc: gleb, rusty, kvm, virtualization, netdev, linux-kernel, markmc,
herbert.xu, quintela, dlaor, avi
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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling
2010-02-23 16:57 ` Michael S. Tsirkin
@ 2010-02-23 19:56 ` Juan Quintela
-1 siblings, 0 replies; 23+ messages in thread
From: Juan Quintela @ 2010-02-23 19:56 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rusty Russell, kvm, virtualization, netdev, linux-kernel,
David Miller, markmc, gleb, herbert.xu, dlaor, avi
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> get_user_pages_fast returns number of pages on success, negative value
> on failure, but never 0. Fix vhost code to match this logic.
It can return 0 if you ask for 0 pages :)
>From the comment:
* Returns number of pages pinned. This may be fewer than the number
* requested. If nr_pages is 0 or negative, returns 0. If no pages
* were pinned, returns -errno.
*/
I agree that code was wrong, but the BUG_ON() is not neccessary
IMHO. The important bit is the change in the comparison.
Reviewed-by: Juan Quintela <quintela@redhat.com>
> 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);
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling
@ 2010-02-23 19:56 ` Juan Quintela
0 siblings, 0 replies; 23+ messages in thread
From: Juan Quintela @ 2010-02-23 19:56 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rusty Russell, kvm, virtualization, netdev, linux-kernel,
David Miller, markmc, gleb, herbert.xu, dlaor, avi
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> get_user_pages_fast returns number of pages on success, negative value
> on failure, but never 0. Fix vhost code to match this logic.
It can return 0 if you ask for 0 pages :)
From the comment:
* Returns number of pages pinned. This may be fewer than the number
* requested. If nr_pages is 0 or negative, returns 0. If no pages
* were pinned, returns -errno.
*/
I agree that code was wrong, but the BUG_ON() is not neccessary
IMHO. The important bit is the change in the comparison.
Reviewed-by: Juan Quintela <quintela@redhat.com>
> 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);
^ permalink raw reply [flat|nested] 23+ messages in thread