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(>->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
Matt Wilson
2013-Nov-05 20:46 UTC
Re: [PATCH] gnttab: fix page_alloc + grant_table deadlock
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(>->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
Jan Beulich
2013-Nov-06 08:57 UTC
Re: [PATCH] gnttab: fix page_alloc + grant_table deadlock
>>> 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