[PATCH] gnttab: fix page_alloc + grant_table deadlock

[PATCH] gnttab: fix page_alloc + grant_table deadlock

[Radim Krčmář]

AB/BA deaclock between d->page_alloc_lock and d->grant_table->lock
happened in this scenario: (names are original)

 gnttab_transfer():               | gnttab_setup_table():
  spin_lock(&e->page_alloc_lock); +  spin_lock(&gt->lock);
  gnttab_prepare_for_transfer():  |  gnttab_grow_table():
                                  |   gnttab_create_shared_page():
                                  |    share_xen_page_with_guest():
    spin_lock(&rgt->lock);        +      spin_lock(&d->page_alloc_lock);

The patch takes grant_table->lock first in gnttab_transfer() and keeps
it longer, which also saves one unlock()+lock().

Based on analysis from Ulrich Obergfell <uobergfe@redhat.com>.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 xen/common/grant_table.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index f42bc7a..fa46a8b 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1434,6 +1434,7 @@ gnttab_query_size(
 /*
  * Check that the given grant reference (rd,ref) allows 'ld' to transfer
  * ownership of a page frame. If so, lock down the grant entry.
+ * rd's grant table lock must be held by the caller.
  */
 static int 
 gnttab_prepare_for_transfer(
@@ -1444,8 +1445,6 @@ gnttab_prepare_for_transfer(
     union grant_combo   scombo, prev_scombo, new_scombo;
     int                 retries = 0;
 
-    spin_lock(&rgt->lock);
-
     if ( rgt->gt_version == 0 )
     {
         gdprintk(XENLOG_INFO,
@@ -1495,11 +1494,9 @@ gnttab_prepare_for_transfer(
         scombo = prev_scombo;
     }
 
-    spin_unlock(&rgt->lock);
     return 1;
 
  fail:
-    spin_unlock(&rgt->lock);
     return 0;
 }
 
@@ -1617,6 +1614,7 @@ gnttab_transfer(
             page = new_page;
         }
 
+        spin_lock(&e->grant_table->lock);
         spin_lock(&e->page_alloc_lock);
 
         /*
@@ -1635,6 +1633,7 @@ gnttab_transfer(
                         "or is dying (%d)\n",
                         e->tot_pages, e->max_pages, gop.ref, e->is_dying);
             spin_unlock(&e->page_alloc_lock);
+            spin_unlock(&e->grant_table->lock);
             rcu_unlock_domain(e);
             put_gfn(d, gop.mfn);
             page->count_info &= ~(PGC_count_mask|PGC_allocated);
@@ -1655,8 +1654,6 @@ gnttab_transfer(
         TRACE_1D(TRC_MEM_PAGE_GRANT_TRANSFER, e->domain_id);
 
         /* Tell the guest about its new page frame. */
-        spin_lock(&e->grant_table->lock);
-
         if ( e->grant_table->gt_version == 1 )
         {
             grant_entry_v1_t *sha = &shared_entry_v1(e->grant_table, gop.ref);
-- 
1.8.4.2


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] gnttab: fix page_alloc + grant_table deadlock

[Matt Wilson]

On Thu, Oct 31, 2013 at 09:29:16PM +0100, Radim Krčmář wrote:
> AB/BA deaclock between d->page_alloc_lock and d->grant_table->lock
> happened in this scenario: (names are original)

"deaclock" -> "deadlock"

>  gnttab_transfer():               | gnttab_setup_table():
>   spin_lock(&e->page_alloc_lock); +  spin_lock(&gt->lock);
>   gnttab_prepare_for_transfer():  |  gnttab_grow_table():
>                                   |   gnttab_create_shared_page():
>                                   |    share_xen_page_with_guest():
>     spin_lock(&rgt->lock);        +      spin_lock(&d->page_alloc_lock);
> 
> The patch takes grant_table->lock first in gnttab_transfer() and keeps
> it longer, which also saves one unlock()+lock().
> 
> Based on analysis from Ulrich Obergfell <uobergfe@redhat.com>.

This is XSA-73. This patch is much smaller than the patches provided
in the advisory. Andrew, can you comment on why your fix should be
applied instead of this one?

--msw

> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  xen/common/grant_table.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index f42bc7a..fa46a8b 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1434,6 +1434,7 @@ gnttab_query_size(
>  /*
>   * Check that the given grant reference (rd,ref) allows 'ld' to transfer
>   * ownership of a page frame. If so, lock down the grant entry.
> + * rd's grant table lock must be held by the caller.
>   */
>  static int 
>  gnttab_prepare_for_transfer(
> @@ -1444,8 +1445,6 @@ gnttab_prepare_for_transfer(
>      union grant_combo   scombo, prev_scombo, new_scombo;
>      int                 retries = 0;
>  
> -    spin_lock(&rgt->lock);
> -
>      if ( rgt->gt_version == 0 )
>      {
>          gdprintk(XENLOG_INFO,
> @@ -1495,11 +1494,9 @@ gnttab_prepare_for_transfer(
>          scombo = prev_scombo;
>      }
>  
> -    spin_unlock(&rgt->lock);
>      return 1;
>  
>   fail:
> -    spin_unlock(&rgt->lock);
>      return 0;
>  }
>  
> @@ -1617,6 +1614,7 @@ gnttab_transfer(
>              page = new_page;
>          }
>  
> +        spin_lock(&e->grant_table->lock);
>          spin_lock(&e->page_alloc_lock);
>  
>          /*
> @@ -1635,6 +1633,7 @@ gnttab_transfer(
>                          "or is dying (%d)\n",
>                          e->tot_pages, e->max_pages, gop.ref, e->is_dying);
>              spin_unlock(&e->page_alloc_lock);
> +            spin_unlock(&e->grant_table->lock);
>              rcu_unlock_domain(e);
>              put_gfn(d, gop.mfn);
>              page->count_info &= ~(PGC_count_mask|PGC_allocated);
> @@ -1655,8 +1654,6 @@ gnttab_transfer(
>          TRACE_1D(TRC_MEM_PAGE_GRANT_TRANSFER, e->domain_id);
>  
>          /* Tell the guest about its new page frame. */
> -        spin_lock(&e->grant_table->lock);
> -
>          if ( e->grant_table->gt_version == 1 )
>          {
>              grant_entry_v1_t *sha = &shared_entry_v1(e->grant_table, gop.ref);

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] gnttab: fix page_alloc + grant_table deadlock

[Jan Beulich]

>>> On 05.11.13 at 21:46, Matt Wilson <msw@linux.com> wrote:
> On Thu, Oct 31, 2013 at 09:29:16PM +0100, Radim Krčmář wrote:
>> The patch takes grant_table->lock first in gnttab_transfer() and keeps
>> it longer, which also saves one unlock()+lock().
>> 
>> Based on analysis from Ulrich Obergfell <uobergfe@redhat.com>.
> 
> This is XSA-73. This patch is much smaller than the patches provided
> in the advisory. Andrew, can you comment on why your fix should be
> applied instead of this one?

I think the reason is quite obvious: Extending a locked region is
always a bad idea if there's an alternative not doing so. The more
so for locks we know can be heavily contended.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel