Re: [PATCH] x86/mm: re-implement get_page_light() using an atomic increment

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

On Mon, Mar 04, 2024 at 08:54:34AM +0100, Jan Beulich wrote:
> On 01.03.2024 13:42, Roger Pau Monne wrote:
> > The current usage of a cmpxchg loop to increase the value of page count is 
> > not
> > optimal on amd64, as there's already an instruction to do an atomic add to a
> > 64bit integer.
> > 
> > Switch the code in get_page_light() to use an atomic increment, as that 
> > avoids
> > a loop construct.  This slightly changes the order of the checks, as current
> > code will crash before modifying the page count_info if the conditions are 
> > not
> > correct, while with the proposed change the crash will happen immediately
> > after having carried the counter increase.  Since we are crashing anyway, I
> > don't believe the re-ordering to have any meaningful impact.
> 
> While I consider this argument fine for ...
> 
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -2580,16 +2580,10 @@ bool get_page(struct page_info *page, const struct 
> > domain *domain)
> >   */
> >  static void get_page_light(struct page_info *page)
> >  {
> > -    unsigned long x, nx, y = page->count_info;
> > +    unsigned long old_pgc = arch_fetch_and_add(&page->count_info, 1);
> >  
> > -    do {
> > -        x  = y;
> > -        nx = x + 1;
> > -        BUG_ON(!(x & PGC_count_mask)); /* Not allocated? */
> 
> ... this check, I'm afraid ...
> 
> > -        BUG_ON(!(nx & PGC_count_mask)); /* Overflow? */
> 
> ... this is a problem unless we discount the possibility of an overflow
> happening in practice: If an overflow was detected only after the fact,
> there would be a window in time where privilege escalation was still
> possible from another CPU. IOW at the very least the description will
> need extending further. Personally I wouldn't chance it and leave this
> as a loop.

So you are worried because this could potentially turn a DoS into an
information leak during the brief period of time where the page
counter has overflowed into the PGC state.

My understating is the BUG_ON() was a mere protection against bad code
that could mess with the counter, but that the counter overflowing is
not a real issue during normal operation.

Thanks, Roger.