Re: [PATCH v5 22/36] mm/mempolicy: add MPOL_F_PRIVATE and zonelist selection

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Private nodes are permanently lost from MPOL_F_STATIC_NODES policies during memory offlining/rebinding.
- [High] Mempolicies targeting private nodes will silently fail to allocate from them due to cpuset restrictions.
- [Medium] Non-atomic clearing of MPOL_F_PRIVATE in pol->flags causes a data race with lockless page allocators.

Pre-existing issues:
- [High] Lockless and unprotected access to pol->nodes in alloc_pages_bulk_interleave can lead to a divide-by-zero crash.
- [High] Unconditional GFP_KERNEL allocation in alloc_pages_bulk_weighted_interleave violates caller's GFP constraints.
--

--- Patch [22]: [PATCH v5 22/36] mm/mempolicy: add MPOL_F_PRIVATE and zonelist selection ---
commit 11224b898bbcf008cfe6641a6cd14bd637edb1d1
Author: Gregory Price <[email protected]>

mm/mempolicy: add MPOL_F_PRIVATE and zonelist selection

This patch adds MPOL_F_PRIVATE to mempolicy flags to identify policies
containing private nodes, allowing private node memory to be reachable.
It also plumbs zonelist selection into the mempolicy allocator interfaces
using the alloc_flags mechanism.

> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 2b76c57a460c9..90110e9761122 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c

[ ... ]

> @@ -505,22 +509,33 @@ static void mpol_rebind_default(struct mempolicy *pol, const nodemask_t *nodes)
>  
>  static void mpol_rebind_nodemask(struct mempolicy *pol, const nodemask_t *nodes)
>  {
> -	nodemask_t tmp;
> +	nodemask_t tmp, priv;
> +
> +	/* preserve online private nodes to re-add later */
> +	nodes_and(priv, pol->nodes, node_states[N_MEMORY_PRIVATE]);

[Severity: High]
Does this evaluate private nodes against pol->nodes instead of 
pol->w.user_nodemask?

If a private node targeted by an MPOL_F_STATIC_NODES policy goes offline, 
it is removed from pol->nodes. When the node comes back online and 
mpol_rebind_nodemask() runs, attempting to restore private nodes from 
pol->nodes will find it empty. 

Could this result in permanent loss of private nodes from static policies?

>  	if (pol->flags & MPOL_F_STATIC_NODES)
>  		nodes_and(tmp, pol->w.user_nodemask, *nodes);

[ ... ]

> -	if (nodes_empty(tmp))
> +	/* private nodes are identity-mapped during remap, drop them here */
> +	nodes_and(tmp, tmp, node_states[N_MEMORY]);
> +	if (nodes_empty(tmp) && nodes_empty(priv))
>  		tmp = *nodes;
>  
> -	pol->nodes = tmp;
> +	/* If any online private nodes remain, add them back */
> +	nodes_or(pol->nodes, tmp, priv);
> +	/* If no online private nodes remain, strip the private flag */
> +	if (nodes_empty(priv))
> +		pol->flags &= ~MPOL_F_PRIVATE;

[Severity: Medium]
Does clearing MPOL_F_PRIVATE non-atomically create a data race with lockless 
page allocators?

When mpol_alloc_flags() concurrently reads (pol->flags & MPOL_F_PRIVATE) 
locklessly in the page allocator fastpath, could this result in torn reads 
and trigger KCSAN warnings?

>  }

[ ... ]

> @@ -2488,7 +2513,7 @@ static struct page *alloc_pages_mpol(gfp_t gfp, unsigned int order,
>  		}
>  	}
>  
> -	page = __alloc_frozen_pages_noprof(gfp, order, nid, nodemask, ALLOC_DEFAULT);
> +	page = __alloc_frozen_pages_noprof(gfp, order, nid, nodemask, aflags);

[Severity: High]
For MPOL_BIND policies, will cpuset_nodemask_valid_mems_allowed() return 
false because private nodes are excluded from cpuset.mems, setting nodemask 
to NULL?

When a NULL nodemask is passed to the allocator, the fastpath defaults to 
cpuset_current_mems_allowed with ALLOC_CPUSET. Since private nodes are 
absent from the cpuset, won't __cpuset_zone_allowed() skip the private 
nodes regardless of ALLOC_ZONELIST_PRIVATE?

Will mempolicies targeting private nodes silently fail to allocate from them 
and fall back to standard nodes?

>  	if (unlikely(pol->mode == MPOL_INTERLEAVE ||

[ ... ]

> @@ -2597,6 +2622,7 @@ static unsigned long alloc_pages_bulk_interleave(gfp_t gfp,
>  		struct mempolicy *pol, unsigned long nr_pages,
>  		struct page **page_array)
>  {
> +	unsigned int aflags = mpol_alloc_flags(pol);
>  	int nodes;
>  	unsigned long nr_pages_per_node;

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to read pol->nodes 
locklessly in alloc_pages_bulk_interleave()?

alloc_pages_bulk_interleave() reads the nodemask word-by-word via 
nodes_weight(pol->nodes) further down in this function.
Concurrently, mpol_rebind_nodemask() mutates pol->nodes word-by-word via 
nodes_or(pol->nodes, tmp, priv).

On systems with >64 nodes, could a torn read result in a weight of 0, 
causing a divide-by-zero panic when calculating 
nr_pages_per_node = nr_pages / nodes?

[ ... ]

> @@ -2632,6 +2658,7 @@ static unsigned long alloc_pages_bulk_weighted_interleave(gfp_t gfp,
>  		struct mempolicy *pol, unsigned long nr_pages,
>  		struct page **page_array)
>  {
> +	unsigned int aflags = mpol_alloc_flags(pol);
>  	struct weighted_interleave_state *state;
>  	struct task_struct *me = current;

[Severity: High]
This is a pre-existing issue, but does alloc_pages_bulk_weighted_interleave() 
violate the caller's GFP constraints?

This function can be called via alloc_pages_bulk_mempolicy_noprof() from 
vmalloc with restrictive flags like GFP_NOFS or GFP_NOIO. However, it 
allocates its weights array unconditionally using 
kzalloc(nr_node_ids, GFP_KERNEL) later in the function.

Could this strip the __GFP_FS or __GFP_IO protection and allow the 
allocation to recurse into the filesystem or block layer, potentially 
causing a deadlock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=22