Re: [PATCH 2/2] xen/mm: do not assign pages to a domain until they are scrubbed

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.03.2026 11:08, Roger Pau Monne wrote:
> ---
> I've attempted various different ways to solve this, but they all ended up
> being impossible.
> 
>  * Prevent non-scrubbed pages from getting extra refcounts (iow: make
>    get_page() fail for them).  This seemed nice, but the cleanup using
>    put_page_alloc_ref() was impossible as non-scrubbed pages would return
>    failure in get_page(), and so I couldn't take the extra reference ahead
>    of calling put_page_alloc_ref().

A special-case variant of get_page() could be introduced, but maybe that
would still be overly fragile.

When we discussed this, what I had proposed didn't require use of get_page()
though. assign_pages() would install two general references (plus one type
ref for PGT_writable) in this special case. To free, you'd call
put_page_alloc_ref() followed by put_page_and_type().

That said, the patch here is still less intrusive than I feared, so I'm not
asking to re-work this again.

>  * Disallow XENMEM_decrease_reservation until the domain has finished
>    creation would fix the issue of pages being freed while pending scrub,
>    but it's not clear there might be other usages that would be problematic,
>    as get_page() on non-scrubbed pages would still return success.

I agree this is of concern.

> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -388,6 +388,12 @@ static void populate_physmap(struct memop_args *a)
>                              goto out;
>                          }
>                      }
> +
> +                    if ( assign_page(page, a->extent_order, d, memflags) )
> +                    {
> +                        free_domheap_pages(page, a->extent_order);

The pages don't have an owner set yet, so that function will go straight
to free_heap_pages(), needlessly passing "true" as last argument. Correct,
but (for large pages, which the stashing is about) highly inefficient.

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2699,7 +2699,13 @@ struct page_info *alloc_domheap_pages(
>                                    memflags, d)) == NULL)) )
>           return NULL;
>  
> -    if ( d && !(memflags & MEMF_no_owner) )
> +    /*
> +     * Don't add pages with the PGC_need_scrub bit set to the domain, the
> +     * caller must clean the bit and then manually call assign_pages().
> +     * Otherwise pages with the PGC_need_scrub would be reachable using
> +     * get_page().
> +     */

How about replacing the latter "with the PGC_need_scrub" by "still subject
to scrubbing"?

> +    if ( d && !(memflags & MEMF_no_owner) && !(memflags & MEMF_keep_scrub) )
>      {
>          if ( memflags & MEMF_no_refcount )
>          {

This no-refcount code isn't repeated at the new call site of assign_page().
It's not needed there, yes, but wouldn't we better allow this to be taken
care of right here, moving the MEMF_keep_scrub check immediately ahead of
the call to assign_page()?

Otherwise should we reject (much earlier) MEMF_no_refcount used together
with MEMF_keep_scrub?

Jan