From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH for-4.5 v11 5/9] xen/arm: Allow hypervisor access to mem_access protected pages Date: Mon, 29 Sep 2014 15:50:23 +0100 Message-ID: <542971AF.1080207@linaro.org> References: <1411990609-22374-1-git-send-email-tklengyel@sec.in.tum.de> <1411990609-22374-6-git-send-email-tklengyel@sec.in.tum.de> <542968E6.2040408@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tamas K Lengyel Cc: wei.liu2@citrix.com, Ian Campbell , Tim Deegan , "xen-devel@lists.xen.org" , Stefano Stabellini , Daniel De Graaf , Tamas K Lengyel List-Id: xen-devel@lists.xenproject.org On 09/29/2014 03:44 PM, Tamas K Lengyel wrote: > > + if ( rc < 0 ) > > + return rc; > > + > > + /* > > + * We do this first as this is faster in the default case when no > > + * permission is set on the page. > > + */ > > + rc = p2m_get_mem_access(current->domain, paddr_to_pfn(ipa), &xma); > > + if ( rc < 0 ) > > + return rc; > > + > > + /* Let's check if mem_access limited the access. */ > > + switch ( xma ) > > + { > > + default: > > + case XENMEM_access_rwx: > > access_rwx is used to say there is no permission, right? If so, why > don't you continue to check permission? > > > So things are backward here. There has already been a MMU fault > (get_page_from_gva failed) and we are trying to determine the cause of > that fault. If the mem_access permission is rwx or rw, that means it had > nothing to do with it so we go back to the original path. This is very confusing. As we should never go there, I would add an ASSERT to catch buggy implementation. > > + ASSERT(*page); > > mfn_to_page only returns a valid pointer if the MFN is valid (see > mfn_valid). > > > Hm, I copied this from get_page_from_gva but I'll remove it. I meant, you forgot to copy the if ( !mfn_valid(mfn) ) return -EFAULT; But the ASSERT is pointless as the function may return an invalid pointer which is not NULL. Regards, -- Julien Grall