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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan