Re: [PATCH v7 03/10] mm/memory_hotplug: pass online_type to online_memory_block() via arg

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incomplete refactoring introduces a TOCTOU race condition where `add_memory_resource` uses a stale snapshot of `online_type` but evaluates the condition using the live `mhp_get_default_online_type()`.
--

--- Patch [3]: [PATCH v7 03/10] mm/memory_hotplug: pass online_type to online_memory_block() via arg ---
commit 72fc58e4b65f508d5057e9fe2ec0ff1c08b34e61
Author: Gregory Price <[email protected]>

mm/memory_hotplug: pass online_type to online_memory_block() via arg

Modify online_memory_block() to accept the online type through its arg
parameter rather than calling mhp_get_default_online_type() internally.

> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 7ac19fab22632..6833208cc17cb 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1494,6 +1496,7 @@ static int create_altmaps_and_memory_blocks(int nid, struct memory_group *group,
>  int add_memory_resource(int nid, struct resource *res, mhp_t mhp_flags)
>  {
>  	struct mhp_params params = { .pgprot = pgprot_mhp(PAGE_KERNEL) };
> +	enum mmop online_type = mhp_get_default_online_type();
>  	enum memblock_flags memblock_flags = MEMBLOCK_NONE;
>  	struct memory_group *group = NULL;
>  	u64 start, size;

[ ... ]

> @@ -1582,7 +1585,8 @@ int add_memory_resource(int nid, struct resource *res, mhp_t mhp_flags)
>  
>  	/* online pages if requested */
>  	if (mhp_get_default_online_type() != MMOP_OFFLINE)
> -		walk_memory_blocks(start, size, NULL, online_memory_block);
> +		walk_memory_blocks(start, size, &online_type,
> +				   online_memory_block);

[Severity: High]
Does checking the live state here instead of the cached online_type introduce
a race condition?

If a concurrent write to the lockless sysfs attribute
/sys/devices/system/memory/auto_online_blocks changes the global policy
from MMOP_OFFLINE to MMOP_ONLINE_MOVABLE while add_memory_resource() is
executing, this condition could evaluate to true while walk_memory_blocks()
uses the cached MMOP_OFFLINE value.

Because the condition uses the live value, but walk_memory_blocks() uses the
cached online_type, could memory_subsys_online() treat MMOP_OFFLINE as an
unconfigured request and forcefully overwrite it to MMOP_ONLINE?

This would result in memory being onlined into the un-removable ZONE_NORMAL
kernel zone instead of being left offline or onlined into ZONE_MOVABLE. This
pins the memory permanently and prevents memory hot-unplug, leading to
resource stranding in virtualized environments.

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