Re: [PATCH v2 for-4.19 1/3] x86/EPT: correct special page checking in epte_get_entry_emt()

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jun 12, 2024 at 04:47:12PM +0200, Jan Beulich wrote:
> On 12.06.2024 16:11, Roger Pau Monné wrote:
> > On Wed, Jun 12, 2024 at 03:16:37PM +0200, Jan Beulich wrote:
> >> mfn_valid() granularity is (currently) 256Mb. Therefore the start of a
> >> 1Gb page passing the test doesn't necessarily mean all parts of such a
> >> range would also pass.
> > 
> > How would such a superpage end up in the EPT?
> > 
> > I would assume this can only happen when adding a superpage MMIO that
> > has part of it return success from mfn_valid()?
> 
> Yes, that's the only way I can think of.
> 
> >> Yet using the result of mfn_to_page() on an MFN
> >> which doesn't pass mfn_valid() checking is liable to result in a crash
> >> (the invocation of mfn_to_page() alone is presumably "just" UB in such a
> >> case).
> >>
> >> Fixes: ca24b2ffdbd9 ("x86/hvm: set 'ipat' in EPT for special pages")
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> > 
> > Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> Thanks.
> 
> >> ---
> >> Of course we could leverage mfn_valid() granularity here to do an
> >> increment by more than 1 if mfn_valid() returned false. Yet doing so
> >> likely would want a suitable helper to be introduced first, rather than
> >> open-coding such logic here.
> > 
> > We would still need to call is_special_page() on each 4K chunk,
> 
> Why? Within any block for which mfn_valid() returns false, there can be
> no RAM pages and hence also no special ones. It's only blocks where
> mfn_valid() returns true that we'd need to iterate through page-by-page.

Oh right, I was thinking the other way around (mfn_valid() returning
true), never mind.

> > at
> > which point taking advantage of the mfn_valid() granularity is likely
> > to make the code more complicated to follow IMO.
> 
> Right, this making it more complicated is the main counter argument. Hence
> why I think that if to go such a route at all, it would need some new
> helper(s) such that at the use sites things still would remain reasonably
> clear.

We could also add an extra check to exit the loop early if special
pages have been found but don't match the current loop index, as it's
all special pages or none.

Thanks, Roger.