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
