[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] x86/PoD: correct ordering of checks in p2m_pod_zero_check()
On 07.04.2020 14:39, Paul Durrant wrote: >> -----Original Message----- >> From: Jan Beulich <jbeulich@xxxxxxxx> >> Sent: 07 April 2020 12:08 >> To: xen-devel@xxxxxxxxxxxxxxxxxxxx >> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Roger Pau Monné >> <roger.pau@xxxxxxxxxx>; Wei Liu >> <wl@xxxxxxx>; Paul Durrant <paul@xxxxxxx> >> Subject: [PATCH] x86/PoD: correct ordering of checks in p2m_pod_zero_check() >> >> Commit 0537d246f8db ("mm: add 'is_special_page' inline function...") >> moved the is_special_page() checks first in its respective changes to >> PoD code. While this is fine for p2m_pod_zero_check_superpage(), the >> validity of the MFN is inferred in both cases from the p2m_is_ram() >> check, which therefore also needs to come first in this 2nd instance. >> >> Take the opportunity and address latent UB here as well - transform >> the MFN into struct page_info * only after having established that >> this is a valid page. >> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx> > > Reviewed-by: Paul Durrant <paul@xxxxxxx> Thanks. > ...with a suggestion below > >> --- >> I will admit that this was build tested only. I did observe the crash >> late yesterday while in the office, but got around to analyzing it only >> today, where I'm again restricted in what I can reasonably test. >> >> --- a/xen/arch/x86/mm/p2m-pod.c >> +++ b/xen/arch/x86/mm/p2m-pod.c >> @@ -877,23 +877,25 @@ p2m_pod_zero_check(struct p2m_domain *p2 >> for ( i = 0; i < count; i++ ) >> { >> p2m_access_t a; >> - struct page_info *pg; >> >> mfns[i] = p2m->get_entry(p2m, gfns[i], types + i, &a, >> 0, NULL, NULL); >> - pg = mfn_to_page(mfns[i]); >> >> /* >> * If this is ram, and not a pagetable or a special page, and >> * probably not mapped elsewhere, map it; otherwise, skip. >> */ >> - if ( !is_special_page(pg) && p2m_is_ram(types[i]) && >> - (pg->count_info & PGC_allocated) && >> - !(pg->count_info & PGC_page_table) && >> - ((pg->count_info & PGC_count_mask) <= max_ref) ) >> - map[i] = map_domain_page(mfns[i]); >> - else >> - map[i] = NULL; >> + map[i] = NULL; >> + if ( p2m_is_ram(types[i]) ) >> + { >> + const struct page_info *pg = mfn_to_page(mfns[i]); > > Perhaps have local scope stack variable for count_info too? I'd view this as useful only if ... >> + >> + if ( !is_special_page(pg) && ... this could then also be made make use of it. Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |