From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [PATCH] xen/p2m: check MFN is in range before using the m2p table Date: Thu, 29 Aug 2013 11:31:21 +0100 Message-ID: <521F22F9.6040004@citrix.com> References: <1377711015-8818-1-git-send-email-david.vrabel@citrix.com> <521F0F5602000078000EF4CA@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VEzWj-0007Bo-9b for xen-devel@lists.xenproject.org; Thu, 29 Aug 2013 10:32:33 +0000 In-Reply-To: <521F0F5602000078000EF4CA@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel , Boris Ostrovsky , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org On 29/08/13 08:07, Jan Beulich wrote: >>>> On 28.08.13 at 19:30, David Vrabel wrote: >> From: David Vrabel >> >> On hosts with more than 168 GB of memory, a 32-bit guest may attempt >> to grant map an MFN that is not cannot lookup in its mapping of the >> m2p table. There is an m2p lookup as part of m2p_add_override() and >> m2p_remove_override(). The lookup falls off the end of the mapped >> portion of the m2p and (because the mapping is at the highest virtual >> address) wraps around and the lookup causes a fault on what appears to >> be a user space address. >> >> do_page_fault() (thinking it's a fault to a userspace address), tries >> to lock mm->mmap_sem. If the gntdev device is used for the grant map, >> m2p_add_override() is called from from gnttab_mmap() with mm->mmap_sem >> already locked. do_page_fault() then deadlocks. >> >> The deadlock would most commonly occur when a 64-bit guest is started >> and xenconsoled attempts to grant map its console ring. >> >> Introduce mfn_to_pfn_no_overrides() which checks the MFN is within the >> mapped portion of the m2p table before accessing the table and use >> this in m2p_add_override(), m2p_remove_override(), and mfn_to_pfn() >> (which already had the correct range check). >> >> All faults caused by accessing the non-existant parts of the m2p are >> thus within the kernel address space and exception_fixup() is called >> without trying to lock mm->mmap_sem. >> >> Signed-off-by: David Vrabel > > This all looks quite fine to me, but iiuc it only removes the deadlock, > it doesn't make things work. In order to make it work I think we'd > need a hypercall (albeit even then it would work only up to 1Tb). > Otoh the question of course is whether driving this big a system > with a 32-bit Dom0 kernel is a reasonable thing in the first place. It should be sufficient as either an MFN is owned by this domain and thus it is within the m2p mapping or it's a foreign MFN and it will be looked up via the m2p overrides. However, I note I got one of the tests wrong in mfn_to_pfn() and it's broken. It should be: pfn = mfn_to_pfn_no_overrides(mfn); if (pfn == ~0 || get_phys_to_machine(pfn) != mfn) pfn = m2p_find_override_pfn(mfn, ~0); Or the equivalent (because get_phys_to_machine(~0) returns INVALID_P2M_ENTRY which is != mfn: pfn = mfn_to_pfn_no_overrides(mfn); if (get_phys_to_machine(pfn) != mfn) pfn = m2p_find_override_pfn(mfn, ~0); David