Re: [PATCH 1/2] xen/mm: don't unconditionally clear PGC_need_scrub in alloc_heap_pages()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.03.2026 11:08, Roger Pau Monne wrote:
> alloc_heap_pages() will unconditionally clear PGC_need_scrub, even when
> MEMF_no_scrub is requested.  This is kind of expected as otherwise some
> callers will assert on seeing non-expected flags set on the count_info
> field.
> 
> Introduce a new MEMF bit to signal to alloc_heap_pages() that non-scrubbed
> pages should keep the PGC_need_scrub bit set. This fixes returning dirty
> pages from alloc_domheap_pages() without the PGC_need_scrub bit set for
> populate_physmap() to consume.
> 
> With the above change alloc_domheap_pages() needs an adjustment to cope
> with allocated pages possibly having the PGC_need_scrub set.
> 
> Fixes: 83a784a15b47 ("xen/mm: allow deferred scrub of physmap populate 
> allocated pages")
> Reported-by: Ayden Bottos <aydenbottos12@xxxxxxxxx>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with one nit (minor request) at the bottom.

> ---
> This issue was initially reported to the Xen Security Team, and it did turn
> out to not require an XSA only because the code hasn't been part of any
> release, otherwise an XSA would have been issued.
> 
> The Security Team would like to thanks Ayden for the prompt report.
> 
> In the scrubbing loop in alloc_heap_pages() i should better be unsigned
> long.

This issue is wider than just that function. As long as MAX_ORDER <= 
BITS_PER_INT,
I think we could have all such loops consistently use unsigned int induction
variables. But of course switching to unsigned long would be okay as well, just
perhaps a little less efficient on (at least) x86. My main wish would be for all
of those variables to be consistent in type (and hence all involved literal
number suffixes also being consistently U or UL).

> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -208,6 +208,8 @@ struct npfec {
>  #define  MEMF_no_refcount (1U<<_MEMF_no_refcount)
>  #define _MEMF_populate_on_demand 1
>  #define  MEMF_populate_on_demand (1U<<_MEMF_populate_on_demand)
> +#define _MEMF_keep_scrub  2
> +#define  MEMF_keep_scrub  (1U<<_MEMF_keep_scrub)
>  #define _MEMF_no_dma      3
>  #define  MEMF_no_dma      (1U<<_MEMF_no_dma)
>  #define _MEMF_exact_node  4

Irrespective of all the similar issues in surrounding code, may I ask that << be
surrounded by blanks in the new addition, to conform to ./CODING_STYLE?

As an aside, I wonder whether we really need the separate _MEMF_keep_scrub, but
the same likely applies to most other _MEMF_*.

Jan