Re: [Xen-devel] [PATCH] gnttab: fix page_alloc + grant_table deadlock

[Matt Wilson <msw@xxxxxxxxx>]

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@xxxxxxxxxx>.

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@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel