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

[Ayden Bottos <aydenbottos12@xxxxxxxxx>]

This looks good to me. I would also add a brief comment in mm.h to
make the contract clearer for future callers: MEMF_keep_scrub is an
internal allocator flag and only valid together with MEMF_no_scrub.

On Thu, Mar 26, 2026 at 12:37 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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