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
