Re: [PATCH v5 06/36] cpuset: exclude private nodes from cpuset.mems (default-open)

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Tasks in descendant cpusets do not have their mempolicies rebound when a private node is hot-unplugged.
- [High] Mempolicy creation (`mbind`, `set_mempolicy`) and page migration (`sys_migrate_pages`) unconditionally reject private nodes, breaking UAPI.
- [Medium] Valid private nodes are unconditionally stripped from tasks' memory policies during any unrelated cpuset update.
--

--- Patch [6]: [PATCH v5 06/36] cpuset: exclude private nodes from cpuset.mems (default-open) ---
commit 9d0d11725cd4a8e139c634034823f38bcc9b8f9f
Author: Gregory Price <[email protected]>

cpuset: exclude private nodes from cpuset.mems (default-open)

This commit modifies cpuset handling to treat N_MEMORY_PRIVATE nodes as globally
accessible resources rather than partitioning them via cpuset.mems. It updates
node validation checks and hotplug logic to accommodate private nodes.

> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index dfd0f827e3b92..05468f95c10bd 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c

[ ... ]

> @@ -3922,9 +3932,12 @@ static void cpuset_handle_hotplug(void)
>  			top_cpuset.mems_allowed = new_mems;
>  		top_cpuset.effective_mems = new_mems;
>  		spin_unlock_irq(&callback_lock);
> -		cpuset_update_tasks_nodemask(&top_cpuset);
>  	}
>  
> +	/* Rebind task mempolicies if any memory node changed state */
> +	if (mems_updated || priv_shrank)
> +		cpuset_update_tasks_nodemask(&top_cpuset);

[Severity: Medium]
When cpuset_update_tasks_nodemask() is called here, it passes 
&cs->mems_allowed to mpol_rebind_mm():

kernel/cgroup/cpuset.c:cpuset_update_tasks_nodemask() {
    ...
    mpol_rebind_mm(mm, &cs->mems_allowed);
}

Because mems_allowed explicitly excludes private nodes, will this
inadvertently strip valid private nodes from a task's static memory policy
during completely unrelated cpuset operations?

> +
>  	mutex_unlock(&cpuset_mutex);
>  
>  	/* if cpus or mems changed, we need to propagate to descendants */
>  	if (cpus_updated || mems_updated) {

[Severity: High]
Does this skip propagating private node hot-unplug events to descendant
cpusets?

If priv_shrank is true but mems_updated is false, this block is skipped.
Could tasks in descendant cpusets retain mempolicies pointing to the newly
offlined private node?

Additionally, if the loop does execute, cpuset_hotplug_update_tasks() 
locally checks:

kernel/cgroup/cpuset.c:cpuset_hotplug_update_tasks() {
    ...
    mems_updated = !nodes_equal(new_mems, cs->effective_mems);
    if (!cpus_updated && !mems_updated)
        goto unlock;
}

Since effective_mems ignores private nodes, it appears descendant updates
would be skipped here as well.

[ ... ]

> @@ -4155,11 +4168,13 @@ nodemask_t cpuset_mems_allowed(struct task_struct *tsk)
>   * cpuset_nodemask_valid_mems_allowed - check nodemask vs. current mems_allowed
>   * @nodemask: the nodemask to be checked
>   *
> - * Are any of the nodes in the nodemask allowed in current->mems_allowed?
> + * Are any of the nodes in the nodemask usable?  N_MEMORY nodes must be in
> + * current->mems_allowed, while N_MEMORY_PRIVATE nodes are always valid.
>   */
>  int cpuset_nodemask_valid_mems_allowed(const nodemask_t *nodemask)
>  {
> -	return nodes_intersects(*nodemask, current->mems_allowed);
> +	return nodes_intersects(*nodemask, current->mems_allowed) ||
> +	       nodes_intersects(*nodemask, node_states[N_MEMORY_PRIVATE]);
>  }

[Severity: High]
Are private nodes unconditionally rejected by sys_migrate_pages and mempolicy
creation?

While cpuset_nodemask_valid_mems_allowed() was updated here, userspace
operations like kernel_migrate_pages() and mpol_set_nodemask() validate
against cpuset_mems_allowed(current), which is defined just above this
function.

cpuset_mems_allowed() delegates to guarantee_online_mems(), which masks
strictly against N_MEMORY.

For example, in mpol_set_nodemask():

mm/mempolicy.c:mpol_set_nodemask() {
    ...
    nodes_and(nsc->mask1, cpuset_current_mems_allowed, node_states[N_MEMORY]);
    ...
}

Does this mean any mbind() or set_mempolicy() targeting an N_MEMORY_PRIVATE
node will still be stripped and return -EINVAL?

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