From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tamas K Lengyel Subject: Re: [PATCH for-4.5 v11 5/9] xen/arm: Allow hypervisor access to mem_access protected pages Date: Mon, 29 Sep 2014 16:57:05 +0200 Message-ID: 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> <542971AF.1080207@linaro.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7415362553616829652==" Return-path: In-Reply-To: <542971AF.1080207@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall 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 --===============7415362553616829652== Content-Type: multipart/alternative; boundary=089e0117746de6110d0504357a03 --089e0117746de6110d0504357a03 Content-Type: text/plain; charset=ISO-8859-1 On Mon, Sep 29, 2014 at 4:50 PM, Julien Grall wrote: > 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. > Not sure what you mean. That's the best approach to really avoid adding any overhead to those cases where mem_access is not in use. We don't even have to check if its in use if get_page_from_gva just works (which I assume is the vast majority of the cases). > > > > + 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. > Ah I see. OK. > > Regards, > > -- > Julien Grall > --089e0117746de6110d0504357a03 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable


On Mon, Sep 29, 2014 at 4:50 PM, Julien Grall <julien.grall@lina= ro.org> wrote:
On 09/29/2014 03:44 PM, Tamas K Lengyel wrote:
>=A0 =A0 =A0> +=A0 =A0 if ( rc < 0 )
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 return rc;
>=A0 =A0 =A0> +
>=A0 =A0 =A0> +=A0 =A0 /*
>=A0 =A0 =A0> +=A0 =A0 =A0* We do this first as this is faster in the= default case when no
>=A0 =A0 =A0> +=A0 =A0 =A0* permission is set on the page.
>=A0 =A0 =A0> +=A0 =A0 =A0*/
>=A0 =A0 =A0> +=A0 =A0 rc =3D p2m_get_mem_access(current->domain, = paddr_to_pfn(ipa), &xma);
>=A0 =A0 =A0> +=A0 =A0 if ( rc < 0 )
>=A0 =A0 =A0> +=A0 =A0 =A0 =A0 return rc;
>=A0 =A0 =A0> +
>=A0 =A0 =A0> +=A0 =A0 /* Let's check if mem_access limited the a= ccess. */
>=A0 =A0 =A0> +=A0 =A0 switch ( xma )
>=A0 =A0 =A0> +=A0 =A0 {
>=A0 =A0 =A0> +=A0 =A0 default:
>=A0 =A0 =A0> +=A0 =A0 case XENMEM_access_rwx:
>
>=A0 =A0 =A0access_rwx is used to say there is no permission, right? If = so, why
>=A0 =A0 =A0don'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 h= ad
> 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<= br> ASSERT to catch buggy implementation.

N= ot sure what you mean. That's the best approach to really avoid adding = any overhead to those cases where mem_access is not in use. We don't ev= en have to check if its in use if get_page_from_gva just w= orks (which I assume is the vast majority of the cases).
=A0

>=A0 =A0 =A0> +=A0 =A0 ASSERT(*page);
>
>=A0 =A0 =A0mfn_to_page only returns a valid pointer if the MFN is valid= (see
>=A0 =A0 =A0mfn_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 -EFAUL= T;

But the ASSERT is pointless as the function may return an invalid
pointer which is not NULL.

Ah I see. OK= .
=A0

Regards,

--
Julien Grall

--089e0117746de6110d0504357a03-- --===============7415362553616829652== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============7415362553616829652==--