[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] Fwd: Question regarding the behavior of guest_physmap_remove_page on x86
On 17/11/15 18:09, Julien Grall wrote: > (Resending with this time xen-devel in CC) > > Hi, > > On ARM, it's possible to fail when removing a page from the P2M. It's > happening if we are trying to shatter a superpage and we don't have > memory to allocate the table. Therefore the mapping won't be removed > from the P2M. > > However on ARM (and until recently on x86 [1]), the function > guest_physmap_remove_page is not supposed to return an error. So we > would free the page even if we fail to remove the page. This will end up > to re-use the page by someone else even though the mapping is still > present in the P2M. > > I looked to the x86 version and I'm not sure how the function is > behaving. Maybe an x86 maintainers could give me insight here. > > I'm thinking to fix the problem by checking the return of > guest_physmap_remove_page to avoid the page being reallocate to someone > else (see for instance guest_remove_page in xen/common/memory.c). Is it > a sensible way to fix it? x86 can just as easily fail because of a failure to shatter a superpage. Despite the below changeset, none of the callee's were updated to actually act upon the error. As a result, the same issue affects x86, in principle. Does ARM have a shadow pool? On x86, we arrange that the shadow pool (should be) large enough so that we never actually encounter an out-of-memory when shattering a superpage. I also observe that there is a latent bug with iommu_unmap_page() (which is part of guest_physmap_remove_page()) as (almost) nothing checks its return value. Currently all (x86) callpaths either return success, or crash the domain. Looking at other codepaths, other possible errors (other than -ENOMEM from shattering) are: if ( unlikely(p2m_is_foreign(p2mt)) ) { /* pvh fixme: foreign types are only supported on ept at present */ gdprintk(XENLOG_WARNING, "Unimplemented foreign p2m type.\n"); return -EINVAL; } or: if ( !(p2m_entry = p2m_find_entry(*table, gfn_remainder, gfn, shift, max)) ) return -ENOENT; All this code looks quite rotten through, and is in some serious need of some error handling hygiene. I would not be surprised if some correctness issues are lurking in here. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |