On 20.06.2022 04:44, Penny Zheng wrote:
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2498,6 +2498,10 @@ void free_domheap_pages(struct page_info *pg, unsigned
> int order)
> }
>
> free_heap_pages(pg, order, scrub);
> +
> + /* Add page on the resv_page_list *after* it has been freed. */
> + if ( unlikely(pg->count_info & PGC_static) )
> + put_static_pages(d, pg, order);
Unless I'm overlooking something the list addition done there / ...
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -90,6 +90,15 @@ void free_staticmem_pages(struct page_info *pg, unsigned
> long nr_mfns,
> bool need_scrub);
> int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int
> nr_mfns,
> unsigned int memflags);
> +#ifdef CONFIG_STATIC_MEMORY
> +#define put_static_pages(d, page, order) ({ \
> + unsigned int i; \
> + for ( i = 0; i < (1 << (order)); i++ ) \
> + page_list_add_tail((pg) + i, &(d)->resv_page_list); \
> +})
... here isn't guarded by any lock. Feels like we've been there before.
It's not really clear to me why the freeing of staticmem pages needs to
be split like this - if it wasn't split, the list addition would
"naturally" occur with the lock held, I think.
Furthermore careful with the local variable name used here. Consider
what would happen with an invocation of
put_static_pages(d, page, i);
To common approach is to suffix an underscore to the variable name.
Such names are not supposed to be used outside of macros definitions,
and hence there's then no potential for such a conflict.
Finally I think you mean (1u << (order)) to be on the safe side against
UB if order could ever reach 31. Then again - is "order" as a parameter
needed here in the first place? Wasn't it that staticmem operations are
limited to order-0 regions?
Jan
