From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v4 2/7] arch, arm: add consistency checks to REMOVE p2m changes Date: Tue, 25 Mar 2014 13:10:39 +0000 Message-ID: <5331804F.3070208@linaro.org> References: <1395712976-19454-1-git-send-email-avanzini.arianna@gmail.com> <1395712976-19454-3-git-send-email-avanzini.arianna@gmail.com> <53317BE7.1080805@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53317BE7.1080805@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: Arianna Avanzini Cc: julien.grall@citrix.com, paolo.valente@unimore.it, keir@xen.org, stefano.stabellini@eu.citrix.com, tim@xen.org, dario.faggioli@citrix.com, Ian.Jackson@eu.citrix.com, xen-devel@lists.xen.org, Ian.Campbell@eu.citrix.com, etrudeau@broadcom.com, JBeulich@suse.com, viktor.kleinik@globallogic.com List-Id: xen-devel@lists.xenproject.org On 03/25/2014 12:51 PM, Julien Grall wrote: > Hi Arianna, > > Thank you for the patch, > > On 03/25/2014 02:02 AM, Arianna Avanzini wrote: >> Currently, the REMOVE case of the switch in apply_p2m_changes() >> does not perform any consistency check on the mapping to be removed. >> More in detail, the code does not check if the guest address to be >> unmapped is actually mapped to the machine address given as a >> parameter. >> This commit attempts to add the above-described consistency check >> to the REMOVE path of apply_p2m_changes(). This is instrumental to >> one of the following commits which implements the possibility to >> trigger the removal of p2m ranges via the memory_mapping DOMCTL >> for ARM. >> >> Signed-off-by: Arianna Avanzini >> Cc: Dario Faggioli >> Cc: Paolo Valente >> Cc: Stefano Stabellini >> Cc: Julien Grall >> Cc: Ian Campbell >> Cc: Jan Beulich >> Cc: Keir Fraser >> Cc: Tim Deegan >> Cc: Ian Jackson >> Cc: Eric Trudeau >> Cc: Viktor Kleinik >> >> --- >> >> v4: >> - Remove useless and slow lookup and use already-available >> data from pte instead. >> - Correctly increment the local variable used to keep the >> machine address whose mapping is currently being removed. >> - Return with an error upon finding a mismatch between the >> actual machine address mapped to the guest address and >> the machine address passed as parameter, instead of just >> skipping the page. >> >> --- >> xen/arch/arm/p2m.c | 24 ++++++++++++++++++++---- >> 1 file changed, 20 insertions(+), 4 deletions(-) >> >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index d00c882..bb0db16 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -243,12 +243,13 @@ static int apply_p2m_changes(struct domain *d, >> int rc; >> struct p2m_domain *p2m = &d->arch.p2m; >> lpae_t *first = NULL, *second = NULL, *third = NULL; >> - paddr_t addr; >> + paddr_t addr, _maddr; >> unsigned long cur_first_page = ~0, >> cur_first_offset = ~0, >> cur_second_offset = ~0; >> unsigned long count = 0; >> unsigned int flush = 0; >> + unsigned long mfn; >> bool_t populate = (op == INSERT || op == ALLOCATE); >> lpae_t pte; >> >> @@ -258,6 +259,7 @@ static int apply_p2m_changes(struct domain *d, >> p2m_load_VTTBR(d); >> >> addr = start_gpaddr; >> + _maddr = maddr; > > You don't need a temporary variable to hold maddr and increment it. > You can directly use maddr, but you will have to remove "maddr += > PAGE_SIZE" in INSERT. > > You also need to increment maddr when first/second level are not valid. > > While reading the code, I'm wondering if we need to return an error if > we are trying to remove a non-existent page. Ian, Stefano, any thoughs? > >> while ( addr < end_gpaddr ) >> { >> if ( cur_first_page != p2m_first_level_index(addr) ) >> @@ -327,6 +329,7 @@ static int apply_p2m_changes(struct domain *d, >> >> flush |= pte.p2m.valid; >> >> + mfn = pte.p2m.base; > > I would move mfn = ... in REMOVE part because it may not be valid on > some place and wrongly used in the future. > >> /* TODO: Handle other p2m type >> * >> * It's safe to do the put_page here because page_alloc will >> @@ -335,8 +338,6 @@ static int apply_p2m_changes(struct domain *d, >> */ >> if ( pte.p2m.valid && p2m_is_foreign(pte.p2m.type) ) >> { >> - unsigned long mfn = pte.p2m.base; >> - >> ASSERT(mfn_valid(mfn)); >> put_page(mfn_to_page(mfn)); >> } >> @@ -367,9 +368,23 @@ static int apply_p2m_changes(struct domain *d, >> maddr += PAGE_SIZE; >> } >> break; >> - case RELINQUISH: >> case REMOVE: >> { >> + ASSERT(pte.p2m.valid); > > Why did you add an ASSERT here? Hmmm ... after reading you patch #4, this assert is wrong. Anyone that can call XEN_DOMCTL_memory_mapping (e.g the toolstack) can crash Xen because there is no check that the GFN has a valid mapping. You should replace this ASSERT by an if ... Regards, -- Julien Grall