| Message ID | 1453897475-19448-1-git-send-email-czuzu@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2016-01-27 at 14:24 +0200, Corneliu ZUZU wrote: > When __p2m_get_mem_access gets called, the p2m lock is already taken > by either get_page_from_gva or p2m_get_mem_access. > > Possible code paths: > 1) -> get_page_from_gva > -> p2m_mem_access_check_and_get_page > -> __p2m_get_mem_access > 2) -> p2m_get_mem_access > -> __p2m_get_mem_access > > In both cases if __p2m_get_mem_access subsequently gets to > call p2m_lookup (happens if !radix_tree_lookup(...)), a hypervisor > hang will occur, since p2m_lookup also spin-locks on the p2m lock. > > This bug-fix simply replaces the p2m_lookup call from > __p2m_get_mem_access > with a call to __p2m_lookup. > > Following Ian's suggestion, we also add an ASSERT to ensure that > the p2m lock is taken upon __p2m_get_mem_access entry. > > Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> Acked + applied, thanks.
On 2/3/2016 1:48 PM, Ian Campbell wrote: > On Wed, 2016-01-27 at 14:24 +0200, Corneliu ZUZU wrote: >> When __p2m_get_mem_access gets called, the p2m lock is already taken >> by either get_page_from_gva or p2m_get_mem_access. >> >> Possible code paths: >> 1) -> get_page_from_gva >> -> p2m_mem_access_check_and_get_page >> -> __p2m_get_mem_access >> 2) -> p2m_get_mem_access >> -> __p2m_get_mem_access >> >> In both cases if __p2m_get_mem_access subsequently gets to >> call p2m_lookup (happens if !radix_tree_lookup(...)), a hypervisor >> hang will occur, since p2m_lookup also spin-locks on the p2m lock. >> >> This bug-fix simply replaces the p2m_lookup call from >> __p2m_get_mem_access >> with a call to __p2m_lookup. >> >> Following Ian's suggestion, we also add an ASSERT to ensure that >> the p2m lock is taken upon __p2m_get_mem_access entry. >> >> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> > Acked + applied, thanks. > > I thought this mail was not sent properly (didn't find it any longer on the web (?)) and I resent it just earlier. I figured it must've been the fact that I forgot to put a "Changed since v1" section & that I didn't include an "--in-reply-to" option. Apparently it was actually sent correctly. Sorry, ignore the last one (which contains a "Changed since v1" section). Corneliu.
On Wed, 2016-02-03 at 13:54 +0200, Corneliu ZUZU wrote: > On 2/3/2016 1:48 PM, Ian Campbell wrote: > > On Wed, 2016-01-27 at 14:24 +0200, Corneliu ZUZU wrote: > > > When __p2m_get_mem_access gets called, the p2m lock is already taken > > > by either get_page_from_gva or p2m_get_mem_access. > > > > > > Possible code paths: > > > 1) -> get_page_from_gva > > > -> p2m_mem_access_check_and_get_page > > > -> __p2m_get_mem_access > > > 2) -> p2m_get_mem_access > > > -> __p2m_get_mem_access > > > > > > In both cases if __p2m_get_mem_access subsequently gets to > > > call p2m_lookup (happens if !radix_tree_lookup(...)), a hypervisor > > > hang will occur, since p2m_lookup also spin-locks on the p2m lock. > > > > > > This bug-fix simply replaces the p2m_lookup call from > > > __p2m_get_mem_access > > > with a call to __p2m_lookup. > > > > > > Following Ian's suggestion, we also add an ASSERT to ensure that > > > the p2m lock is taken upon __p2m_get_mem_access entry. > > > > > > Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> > > Acked + applied, thanks. > > > > > I thought this mail was not sent properly (didn't find it any longer on > the web (?)) and I resent it just earlier. > I figured it must've been the fact that I forgot to put a "Changed since > v1" section & that I didn't include an > "--in-reply-to" option. Apparently it was actually sent correctly. > Sorry, ignore the last one (which contains a "Changed since v1" section). OK, please check that what is currently in xen.git#staging is what you think should be there. Ian.
On 2/3/2016 2:23 PM, Ian Campbell wrote: > On Wed, 2016-02-03 at 13:54 +0200, Corneliu ZUZU wrote: >> I thought this mail was not sent properly (didn't find it any longer on >> the web (?)) and I resent it just earlier. >> I figured it must've been the fact that I forgot to put a "Changed since >> v1" section & that I didn't include an >> "--in-reply-to" option. Apparently it was actually sent correctly. >> Sorry, ignore the last one (which contains a "Changed since v1" section). > OK, please check that what is currently in xen.git#staging is what you > think should be there. > > Ian. Yep, just checked, the changes are there. Corneliu.
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 2190908..e8e6db4 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -468,6 +468,8 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t gfn, #undef ACCESS }; + ASSERT(spin_is_locked(&p2m->lock)); + /* If no setting was ever set, just return rwx. */ if ( !p2m->mem_access_enabled ) { @@ -490,7 +492,7 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t gfn, * No setting was found in the Radix tree. Check if the * entry exists in the page-tables. */ - paddr_t maddr = p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL); + paddr_t maddr = __p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL); if ( INVALID_PADDR == maddr ) return -ESRCH;
When __p2m_get_mem_access gets called, the p2m lock is already taken by either get_page_from_gva or p2m_get_mem_access. Possible code paths: 1) -> get_page_from_gva -> p2m_mem_access_check_and_get_page -> __p2m_get_mem_access 2) -> p2m_get_mem_access -> __p2m_get_mem_access In both cases if __p2m_get_mem_access subsequently gets to call p2m_lookup (happens if !radix_tree_lookup(...)), a hypervisor hang will occur, since p2m_lookup also spin-locks on the p2m lock. This bug-fix simply replaces the p2m_lookup call from __p2m_get_mem_access with a call to __p2m_lookup. Following Ian's suggestion, we also add an ASSERT to ensure that the p2m lock is taken upon __p2m_get_mem_access entry. Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> --- xen/arch/arm/p2m.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)