Re: [PATCH v5 25/36] mm/memory_hotplug: support N_MEMORY_PRIVATE node hotplug

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Private node registration and unregistration incorrectly rely on N_MEMORY and N_MEMORY_PRIVATE states, which only track online memory, causing isolation breakage and hijacking of normal memory nodes.
- [High] A TOCTOU race condition exists between memory unplug and private node registration due to lock ordering.
- [Medium] Direct access of __rcu pointer in pgdat_is_private() without using rcu_access_pointer().
--

--- Patch [25]: [PATCH v5 25/36] mm/memory_hotplug: support N_MEMORY_PRIVATE node hotplug ---
commit 976d363a06939b47fdc2d106fc8f3ec46359abd3
Author: Gregory Price <[email protected]>
Subject: mm/memory_hotplug: support N_MEMORY_PRIVATE node hotplug

This commit introduces add_private_memory_driver_managed() to allow
modules to hotplug memory into an N_MEMORY_PRIVATE node. It introduces
logic to register node ownership and opts these private nodes out of
reclaim/compaction by default.

> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 2b9c0830821e6..be230ac9efe5a 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c

[ ... ]

> @@ -1204,13 +1204,20 @@ int online_pages(unsigned long pfn, unsigned long nr_pages,
>  	online_pages_range(pfn, nr_pages);
>  	adjust_present_page_count(pfn_to_page(pfn), group, nr_pages);
>  
> +	/*
> +	 * N_MEMORY and N_MEMORY_PRIVATE are mutually exclusive, determine
> +	 * which is correct based on whether the pgdat->private is set.
> +	 */
>  	if (node_arg.nid >= 0)
> -		node_set_state(nid, N_MEMORY);
> +		node_set_state(nid, pgdat_is_private(NODE_DATA(nid)) ?
> +				    N_MEMORY_PRIVATE : N_MEMORY);

[Severity: Medium]
Does pgdat_is_private() directly evaluate the __rcu annotated
pgdat->node_private pointer without using rcu_access_pointer()? A direct
boolean evaluation of an __rcu pointer can result in compiler load tearing.

[ ... ]

> @@ -1688,6 +1702,19 @@ int __add_memory_driver_managed(int nid, u64 start, u64 size,
>  	if (online_type < MMOP_OFFLINE || online_type > MMOP_ONLINE_MOVABLE)
>  		return -EINVAL;
>  
> +	/* Register a private-node owner before adding memory. */
> +	if (np) {
> +		if (mhp_flags & MHP_NID_IS_MGID) {
> +			group = memory_group_find_by_id(nid);
> +			if (!group)
> +				return -EINVAL;
> +			real_nid = group->nid;
> +		}
> +		rc = node_private_register(real_nid, np);
> +		if (rc)
> +			return rc;
> +	}
> +
>  	lock_device_hotplug();

[Severity: High]
Is there a race condition here since node_private_register() is called before
acquiring device_hotplug_lock? 

If Thread A drops the lock in offline_and_remove_memory_ranges() before
calling node_private_unregister(), Thread B could successfully register
the node here. Thread B then blocks on device_hotplug_lock, allowing Thread A
to clear the registration, which might cause Thread B to online its newly
added private memory incorrectly as normal system RAM.

[ ... ]

> @@ -1702,10 +1729,41 @@ int __add_memory_driver_managed(int nid, u64 start, u64 size,
>  
>  out_unlock:
>  	unlock_device_hotplug();
> +	if (rc < 0 && np)
> +		node_private_unregister(real_nid);
>  	return rc;
>  }

[Severity: High]
If this error path is hit, does node_private_unregister() unconditionally
drop the private node registration based only on the online state, ignoring
offline memory blocks on the same node? This could break previously added
offline blocks on this node.

[ ... ]

> @@ -2535,6 +2610,10 @@ int offline_and_remove_memory_ranges(const struct range *ranges,
>  out_unlock:
>  	unlock_device_hotplug();
>  
> +	/* Drop private node registration if they are now memoryless */
> +	for_each_node_mask(nid, nodes)
> +		node_private_unregister(nid);
> +
>  	kfree(online_types);
>  	return rc;
>  }

[Severity: High]
Could node_private_unregister() blindly clear pgdat->node_private here
if it only checks the online N_MEMORY state instead of present memory? 
If remaining memblocks on the node are offline, they might become normal
system RAM when later onlined.

Additionally, because device_hotplug_lock is dropped before unregistering
the private nodes, does this open a race window where a concurrent 
add_private_memory_driver_managed() could have its state wiped out?

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