Re: [PATCH v2 4/4] mm: hugetlb: fix cpuset violations in gigantic surplus allocations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] mm: hugetlb: empty nodemask passed in alloc_buddy_hugetlb_folio()
- [Medium] mm: hugetlb: MPOL_BIND violation in alloc_buddy_hugetlb_folio() on cpuset race
--

commit 6e27dfcfc4f6238bea6b6e8639c15188d8d94295
Author: Qi Zheng <qi.zheng@linux.dev>

mm: hugetlb: fix cpuset violations in gigantic surplus allocations

This commit intersects the allocation nodemask in alloc_buddy_hugetlb_folio()
with cpuset_current_mems_allowed so gigantic surplus hugepage allocations do
not allocate from nodes outside the current task's cpuset. It also retries
failed allocations if cpuset_current_mems_allowed changes during the attempt.

> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 6fba92b0f6749..09b64f7c11dd2 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -2181,22 +2181,33 @@ struct folio *alloc_buddy_hugetlb_folio(struct hstate *h,
>  		gfp_t gfp_mask, struct mempolicy_interpreted *mpoli)
>  {
>  	struct folio *folio = NULL;
> -	nodemask_t *nodemask = mpoli->nodemask;
> +	nodemask_t alloc_nodemask;
> +	unsigned int cpuset_mems_cookie;
> +
> +retry:
> +	cpuset_mems_cookie = read_mems_allowed_begin();
> +	alloc_nodemask = cpuset_current_mems_allowed;
> +	if (mpoli->mode == MPOL_BIND && mpoli->nodemask &&
> +	    nodes_intersects(*mpoli->nodemask, alloc_nodemask))
> +		nodes_and(alloc_nodemask, alloc_nodemask, *mpoli->nodemask);
>  
>  	if (mpoli->mode == MPOL_PREFERRED_MANY) {
>  		gfp_t gfp = gfp_mask & ~(__GFP_DIRECT_RECLAIM | __GFP_NOFAIL);
>  
> +		nodes_and(alloc_nodemask, alloc_nodemask, *mpoli->nodemask);
>  		folio = alloc_surplus_hugetlb_folio(h, gfp, mpoli->nid,
> -						    nodemask);
> +						    &alloc_nodemask);

[Severity: Medium]
What happens here when an MPOL_PREFERRED_MANY policy's nodemask is disjoint
from cpuset_current_mems_allowed (for example, after moving a task or
non-leader thread to a cpuset on different NUMA nodes, since
mpol_rebind_preferred() does not remap pol->nodes)?

Unlike the MPOL_BIND branch above, this path does not check whether the
intersection is non-empty before calling alloc_surplus_hugetlb_folio() with
&alloc_nodemask. When alloc_nodemask is empty:

1. For gigantic hugepages, alloc_surplus_hugetlb_folio() performs a wasted
   pass through CMA and alloc_contig_frozen_pages() with an empty nodemask.

2. For non-gigantic hugepages, alloc_surplus_hugetlb_folio() ->
   alloc_fresh_hugetlb_folio() -> only_alloc_fresh_hugetlb_folio() ->
   alloc_buddy_frozen_folio() -> __alloc_frozen_pages_noprof() fails the fast
   path and enters __alloc_pages_slowpath(). In check_retry_cpuset():

   mm/page_alloc.c:check_retry_cpuset() {
   	if (cpusets_enabled() && ac->nodemask &&
   			!cpuset_nodemask_valid_mems_allowed(ac->nodemask)) {
   		ac->nodemask = NULL;
   		return true;
   	}
   }

   Because cpuset_nodemask_valid_mems_allowed() returns false for an empty
   nodemask, check_retry_cpuset() treats it as a raced MPOL_BIND nodemask,
   resets ac->nodemask to NULL, and restarts __alloc_pages_slowpath() with
   __GFP_DIRECT_RECLAIM masked off. That wakes kswapd across all nodes via
   wake_all_kswapds() with ac->nodemask == NULL, attempts an allocation on
   non-preferred fallback cpuset nodes at ALLOC_WMARK_MIN before the normal
   fallback pass at ALLOC_WMARK_LOW with reclaim/compaction runs, and
   increments HTLB_BUDDY_PGALLOC_FAIL in alloc_buddy_frozen_folio() if that
   first pass fails.

Should the first-pass alloc_surplus_hugetlb_folio() call be guarded by
if (nodes_and(alloc_nodemask, alloc_nodemask, *mpoli->nodemask)) so that an
empty intersection skips straight to the cpuset fallback pass?

>  
> -		/* Fallback to all nodes if page==NULL */
> -		nodemask = NULL;
> +		/* Fallback to all nodes allowed by the current cpuset */
> +		alloc_nodemask = cpuset_current_mems_allowed;
>  	}
>  
> -	if (!folio) {
> +	if (!folio)
>  		folio = alloc_surplus_hugetlb_folio(h, gfp_mask, mpoli->nid,
> -						    nodemask);
> -	}
> +						    &alloc_nodemask);
> +
> +	if (!folio && unlikely(read_mems_allowed_retry(cpuset_mems_cookie)))
> +		goto retry;

[Severity: Medium]
Can pre-intersecting alloc_nodemask with cpuset_current_mems_allowed cause
non-gigantic surplus allocations to violate MPOL_BIND or use a stale nodemask
when cpuset_current_mems_allowed changes concurrently?

alloc_buddy_hugetlb_folio() is used for both gigantic and non-gigantic
surplus hugepage allocations. For non-gigantic hugepages,
alloc_surplus_hugetlb_folio() calls into __alloc_frozen_pages_noprof() ->
__alloc_pages_slowpath() -> check_retry_cpuset(), which expects ac->nodemask
to be the raw policy nodemask rather than a nodemask pre-intersected with an
earlier snapshot of cpuset_current_mems_allowed:

1. Suppose a VMA MPOL_BIND policy has *mpoli->nodemask = {0, 2} and
   current->mems_allowed is initially {0}, so alloc_nodemask is computed as
   {0}. If current->mems_allowed changes to {1, 2} while the thread enters
   __alloc_pages_slowpath(), check_retry_cpuset() sees that ac->nodemask ({0})
   no longer intersects current->mems_allowed ({1, 2}), even though the raw
   MPOL_BIND mask ({0, 2}) still intersects {1, 2} on node 2.
   check_retry_cpuset() then clears ac->nodemask = NULL and restarts the
   slowpath, which can allocate a folio on node 1 (allowed by the new cpuset
   {1, 2}, but forbidden by MPOL_BIND {0, 2}). Because folio is non-NULL,
   !folio && unlikely(read_mems_allowed_retry(cpuset_mems_cookie)) does not
   trigger a retry, and the folio from node 1 is returned.

2. In the MPOL_BIND check at the top of alloc_buddy_hugetlb_folio():

   	if (mpoli->mode == MPOL_BIND && mpoli->nodemask &&
   	    nodes_intersects(*mpoli->nodemask, alloc_nodemask))
   		nodes_and(alloc_nodemask, alloc_nodemask, *mpoli->nodemask);

   *mpoli->nodemask is read twice. If mpoli->nodemask points to a task
   mempolicy (current->mempolicy->nodes), a concurrent
   cpuset_change_task_nodemask() -> mpol_rebind_task() ->
   mpol_rebind_nodemask() can update pol->nodes between nodes_intersects()
   and nodes_and(), leaving alloc_nodemask empty.

3. For MPOL_DEFAULT (where nodemask was previously NULL), passing
   &alloc_nodemask (for example {0, 1}) into __alloc_frozen_pages_noprof()
   means that if current->mems_allowed changes to {1, 2} during
   __alloc_pages_slowpath(), check_retry_cpuset() sees a non-empty
   intersection on node 1 and loops in the slowpath with ac->nodemask still
   restricted to {0, 1}, preventing slowpath retries from considering newly
   allowed node 2.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793850539.git.zhengqi.arch@bytedance.com?part=4