Xen devel - Nov 2013 - [PATCH] gnttab: lock the left grant table earlier in __gnttab_unmap_common()

From: Matt Wilson <msw@amazon.com>

Luckily today maptrack_limit never shrinks. But if at some point in
the future this were to change, checking maptrack_limit without
holding the grant table spinlock would no longer be safe.

Cc: xen-devel@lists.xenproject.org
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Anthony Liguori <aliguori@amazon.com>
Signed-off-by: Matt Wilson <msw@amazon.com>
---
 xen/common/grant_table.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 21c6a14..ef10ff4 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -842,15 +842,16 @@ __gnttab_unmap_common(
 
     op->frame = (unsigned long)(op->dev_bus_addr >> PAGE_SHIFT);
 
+    spin_lock(&lgt->lock);
     if ( unlikely(op->handle >= lgt->maptrack_limit) )
     {
+        spin_unlock(&lgt->lock);
         gdprintk(XENLOG_INFO, "Bad handle (%d).\n", op->handle);
         op->status = GNTST_bad_handle;
         return;
     }
 
     op->map = &maptrack_entry(lgt, op->handle);
-    spin_lock(&lgt->lock);
 
     if ( unlikely(!op->map->flags) )
     {
-- 
1.7.9.5

On Sun, Nov 10, 2013 at 6:07 PM, Matt Wilson <msw@linux.com>
wrote:
> From: Matt Wilson <msw@amazon.com>
>
> Luckily today maptrack_limit never shrinks. But if at some point in
> the future this were to change, checking maptrack_limit without
> holding the grant table spinlock would no longer be safe.
Reviewed-by: Anthony Liguori <aliguori@amazon.com>

The commit doesn''t mention it, but this problem could cause the
function to fail with "Bad handle" even when the handle is valid (in
theory at least).

If the maptrack table could shrink then it would be a overflow.

Regards,

Anthony Liguori
>
> Cc: xen-devel@lists.xenproject.org
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Anthony Liguori <aliguori@amazon.com>
> Signed-off-by: Matt Wilson <msw@amazon.com>
> ---
>  xen/common/grant_table.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 21c6a14..ef10ff4 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -842,15 +842,16 @@ __gnttab_unmap_common(
>
>      op->frame = (unsigned long)(op->dev_bus_addr >>
PAGE_SHIFT);
>
> +    spin_lock(&lgt->lock);
>      if ( unlikely(op->handle >= lgt->maptrack_limit) )
>      {
> +        spin_unlock(&lgt->lock);
>          gdprintk(XENLOG_INFO, "Bad handle (%d).\n",
op->handle);
>          op->status = GNTST_bad_handle;
>          return;
>      }
>
>      op->map = &maptrack_entry(lgt, op->handle);
> -    spin_lock(&lgt->lock);
>
>      if ( unlikely(!op->map->flags) )
>      {
> --
> 1.7.9.5
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

On 11/11/13 02:07, Matt Wilson wrote:
> From: Matt Wilson <msw@amazon.com>
>
> Luckily today maptrack_limit never shrinks. But if at some point in
> the future this were to change, checking maptrack_limit without
> holding the grant table spinlock would no longer be safe.
>
> Cc: xen-devel@lists.xenproject.org
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Anthony Liguori <aliguori@amazon.com>
> Signed-off-by: Matt Wilson <msw@amazon.com>
> ---
>  xen/common/grant_table.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 21c6a14..ef10ff4 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -842,15 +842,16 @@ __gnttab_unmap_common(
>  
>      op->frame = (unsigned long)(op->dev_bus_addr >>
PAGE_SHIFT);
>  
> +    spin_lock(&lgt->lock);
>      if ( unlikely(op->handle >= lgt->maptrack_limit) )
>      {
> +        spin_unlock(&lgt->lock);
>          gdprintk(XENLOG_INFO, "Bad handle (%d).\n",
op->handle);
>          op->status = GNTST_bad_handle;
>          return;
>      }
>  
>      op->map = &maptrack_entry(lgt, op->handle);
> -    spin_lock(&lgt->lock);
>  
>      if ( unlikely(!op->map->flags) )
>      {

>>> On 11.11.13 at 03:07, Matt Wilson <msw@linux.com> wrote:
> Luckily today maptrack_limit never shrinks. But if at some point in
"Luckily" == intentionally.
> the future this were to change, checking maptrack_limit without
> holding the grant table spinlock would no longer be safe.
Are you convinced that this is the only issue with the limit
shrinking? I''m not, and therefore fixing the issue here seems
rather pointless to me.

Jan

>>> On 11.11.13 at 03:14, Anthony Liguori <anthony@codemonkey.ws>
wrote:
> On Sun, Nov 10, 2013 at 6:07 PM, Matt Wilson <msw@linux.com> wrote:
>> From: Matt Wilson <msw@amazon.com>
>>
>> Luckily today maptrack_limit never shrinks. But if at some point in
>> the future this were to change, checking maptrack_limit without
>> holding the grant table spinlock would no longer be safe.
> 
> Reviewed-by: Anthony Liguori <aliguori@amazon.com>
> 
> The commit doesn''t mention it, but this problem could cause the
> function to fail with "Bad handle" even when the handle is valid
(in
> theory at least).
But only if the growing of the table races the operation here.
Which ought to be impossible, as we''re dealing with an unmap
here (i.e. the handle must have been established earlier). And
even if not impossible, it''s pointless - checking against the
lower (pre-growth) value is fine here, as guests are supposed
to not issue requests resulting in growth in parallel with
requests utilizing the grown table (and they also should be
unable to, as they can''t possibly know of the "bigger"
handle,
which would be returned to them only after the growing
completed).

Or else - what am I overlooking?

Jan

On 11/11/13 02:07, Matt Wilson wrote:
> From: Matt Wilson <msw@amazon.com>
> 
> Luckily today maptrack_limit never shrinks. But if at some point in
> the future this were to change, checking maptrack_limit without
> holding the grant table spinlock would no longer be safe.
I don''t think we should extend region the grant table lock protects if
it''s not needed since we know this lock is heavily contended.

Also, doesn''t the ''l'' and ''r''
prefixes mean ''local'' and ''remote'' not
''left'' and ''right''?

David

On Mon, Nov 11, 2013 at 2:53 AM, David Vrabel <david.vrabel@citrix.com>
wrote:
> On 11/11/13 02:07, Matt Wilson wrote:
>> From: Matt Wilson <msw@amazon.com>
>>
>> Luckily today maptrack_limit never shrinks. But if at some point in
>> the future this were to change, checking maptrack_limit without
>> holding the grant table spinlock would no longer be safe.
>
> I don''t think we should extend region the grant table lock
protects if
> it''s not needed since we know this lock is heavily contended.
It is heavily contended.  So heavily contended that with a high number
of disks (20+) you lose 50% of the potential IOPS just because of the
grant lock.  Persistent grants paper over this problem but with older
guests there''s no way to avoid it.

A little critical section like this isn''t going to move the scale very
much and correctness should always trump performance.

Letting a guest confuse the hypervisor is a recipe for an XSA.  I
could appreciate a suggestion to replace the change with a comment
explaining why we''re allowing inconsistency but letting it be silently
broken is not reasonable IMHO.

Just keeping a few instructions out of the critical section isn''t
going to fix the 50% degradation.  Fortunately, Matt has been working
on this problem and that''s why he is poking at this code. 
He''s got a
patch that splits the grant lock and we get back all of the lost
performance.  Matt, perhaps you can post a WIP version of the patch?
> Also, doesn''t the ''l'' and ''r''
prefixes mean ''local'' and ''remote'' not
> ''left'' and ''right''?
I know since the great bit shortage of ''05, people have been using
obscure three variable names to conserve bits.  But these days, we can
resolve conflicts like this by just using variable names like
local_grant_table or left_grant_table ;-)

Regards,

Anthony Liguori
>
> David
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

On Mon, Nov 11, 2013 at 1:58 AM, Jan Beulich <JBeulich@suse.com>
wrote:
>>>> On 11.11.13 at 03:14, Anthony Liguori
<anthony@codemonkey.ws> wrote:
>> On Sun, Nov 10, 2013 at 6:07 PM, Matt Wilson <msw@linux.com>
wrote:
>>> From: Matt Wilson <msw@amazon.com>
>>>
>>> Luckily today maptrack_limit never shrinks. But if at some point in
>>> the future this were to change, checking maptrack_limit without
>>> holding the grant table spinlock would no longer be safe.
>>
>> Reviewed-by: Anthony Liguori <aliguori@amazon.com>
>>
>> The commit doesn''t mention it, but this problem could cause
the
>> function to fail with "Bad handle" even when the handle is
valid (in
>> theory at least).
>
> But only if the growing of the table races the operation here.
> Which ought to be impossible, as we''re dealing with an unmap
> here (i.e. the handle must have been established earlier). And
> even if not impossible, it''s pointless - checking against the
> lower (pre-growth) value is fine here, as guests are supposed
> to not issue requests resulting in growth in parallel with
> requests utilizing the grown table
There are no locks here so in between dropping the grant table lock,
anything can happen in theory including table growth and a larger
handle return.  I have seen horribly broken systems out there with SMM
routines that can run for almost a millisecond.

I agree, it''s not at all practical and extremely unlikely to happen in
practice.  That''s why I qualified the comment with "in theory at
least".

Regards,

Anthony Liguori
> (and they also should be
> unable to, as they can''t possibly know of the "bigger"
handle,
> which would be returned to them only after the growing
> completed).
>
> Or else - what am I overlooking?
>
> Jan
>

>>> On 11.11.13 at 15:52, Anthony Liguori <anthony@codemonkey.ws>
wrote:
> I agree, it''s not at all practical and extremely unlikely to
happen in
> practice.  That''s why I qualified the comment with "in theory
at
> least".
Extremely unlikely still means not impossible. And hence necessary
to be fixed. Yet my question stands - can this really happen with a
well behaved guest? As said, I think we''re fine with a guest issuing
racing calls getting what it deserves, so long as the hypervisor or
other guests aren''t affected.

Jan

On 11/11/13 14:48, Anthony Liguori wrote:
> Fortunately, Matt has been working
> on this problem and that''s why he is poking at this code. 
He''s got a
> patch that splits the grant lock and we get back all of the lost
> performance.  Matt, perhaps you can post a WIP version of the patch?
Please.  Or if it''s not ready yet, a summary of the approach taken
would
be interesting.

David

On Mon, Nov 11, 2013 at 8:23 AM, Jan Beulich <JBeulich@suse.com>
wrote:
>>>> On 11.11.13 at 15:52, Anthony Liguori
<anthony@codemonkey.ws> wrote:
>> I agree, it''s not at all practical and extremely unlikely to
happen in
>> practice.  That''s why I qualified the comment with "in
theory at
>> least".
>
> Extremely unlikely still means not impossible. And hence necessary
> to be fixed. Yet my question stands - can this really happen with a
> well behaved guest?
I''m not sure to be perfectly honest.  It seems like it would be
necessary to predict a future grant handle to trigger this.
> As said, I think we''re fine with a guest issuing
> racing calls getting what it deserves, so long as the hypervisor or
> other guests aren''t affected.
I''m less concerned about what happens to the guest than I am about
having the hypervisor be confused.

I can also see someone looking at the code and seeing this check not
guarded by a lock and drawing the conclusion that locking is not
necessary in other places where the result could be a security
concern.

Regards,

Anthony Liguori
> Jan
>

On Mon, Nov 11, 2013 at 10:53:23AM +0000, David Vrabel
wrote:
> On 11/11/13 02:07, Matt Wilson wrote:
> > From: Matt Wilson <msw@amazon.com>
> > 
> > Luckily today maptrack_limit never shrinks. But if at some point in
> > the future this were to change, checking maptrack_limit without
> > holding the grant table spinlock would no longer be safe.
> 
> I don''t think we should extend region the grant table lock
protects if
> it''s not needed since we know this lock is heavily contended.
I''ve been looking into the grant table lock contention. Moving this up
didn''t degrade performance in the highly contended test scenario
I''ve
set up.

This change makes more sense when put in the context of some locking
changes I''ve been experimenting with.
> Also, doesn''t the ''l'' and ''r''
prefixes mean ''local'' and ''remote'' not
> ''left'' and ''right''?
Oops. As an aside, I''ve never liked the "remote" designation.
The
common use of "foreign" also makes this confusing. Every time I draw a
dom0 <-> domU diagram on a whiteboard dom0 is on the left, leading me
to this mistake. ;-)

I''ll update the description.

--msw

From: Matt Wilson <msw@amazon.com>

Luckily today maptrack_limit never shrinks. But if at some point in
the future this were to change, checking maptrack_limit without
holding the grant table spinlock would no longer be safe.

Cc: xen-devel@lists.xenproject.org
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Anthony Liguori <aliguori@amazon.com>
Signed-off-by: Matt Wilson <msw@amazon.com>

---
v1->v2:
* updated summary to use "local" instead of "left"

 xen/common/grant_table.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 21c6a14..ef10ff4 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -842,15 +842,16 @@ __gnttab_unmap_common(
 
     op->frame = (unsigned long)(op->dev_bus_addr >> PAGE_SHIFT);
 
+    spin_lock(&lgt->lock);
     if ( unlikely(op->handle >= lgt->maptrack_limit) )
     {
+        spin_unlock(&lgt->lock);
         gdprintk(XENLOG_INFO, "Bad handle (%d).\n", op->handle);
         op->status = GNTST_bad_handle;
         return;
     }
 
     op->map = &maptrack_entry(lgt, op->handle);
-    spin_lock(&lgt->lock);
 
     if ( unlikely(!op->map->flags) )
     {
-- 
1.7.4.5