[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
Hi Andrew, On 17/11/15 18:47, Andrew Cooper wrote: > On 17/11/15 18:09, Julien Grall wrote: >> 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. We don't have shadow pool on ARM. Even if we implement I think we have to check the return of guest_physmap_remove_page in the event there is other error path. > 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 don't know the x86 code. I would appreciate can take care of the x86 part. Regards, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |