Re: [PATCH v4 5/9] mm/memory_hotplug: add multi-range hotunplug

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] If `try_remove_memory()` fails during Phase 2, the rollback phase irrevocably corrupts the state array and breaks the 'all-or-nothing' guarantee.
--

commit 8e30dd8a7c852dc51179b28a0c0a3841cc1be525
Author: Gregory Price <gourry@gourry.net>

mm/memory_hotplug: add multi-range hotunplug

This commit adds offline_and_remove_memory_ranges() to offline and remove
multiple memory ranges atomically. It offlines all ranges in phase 1, and
removes them in phase 2, falling back to restoring online types if failures
occur.

> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 7d145217adc6b..e486d35c22b2c 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c

[ ... ]

> +	/*
> +	 * Phase 2: only once everything is offline, remove it.  This cannot
> +	 * fail as the memory can no longer be onlined in the meantime.
> +	 */
> +	if (!rc) {
> +		for (i = 0; i < nr_ranges; i++) {
> +			rc = try_remove_memory(ranges[i].start,
> +					       range_len(&ranges[i]));
> +			if (rc) {
> +				pr_err("%s: Failed to remove memory: %d",
> +				       __func__, rc);
> +				break;
> +			}
> +		}
> +	}

[Severity: High]
The comment states that this cannot fail, but try_remove_memory() can return
errors, for example if memory_blocks_have_altmaps() returns -EINVAL.

If try_remove_memory() succeeds for the first range but fails for a subsequent
range, the blocks for the first range are irrevocably removed. Does this leave
the system in a partially removed state, breaking the all-or-nothing guarantee?

> +
> +	/*
> +	 * Roll back the offlining if anything failed.  Blocks we never offlined
> +	 * are marked MMOP_OFFLINE and skipped by try_reonline_memory_block().
> +	 */
> +	if (rc) {
> +		tmp = online_types;
> +		for (i = 0; i < nr_ranges; i++)
> +			walk_memory_blocks(ranges[i].start,
> +					   range_len(&ranges[i]), &tmp,
> +					   try_reonline_memory_block);
> +	}

[Severity: High]
If we enter this rollback path after a partial failure in phase 2, will the
state array tracking become corrupted?

Since the first range was physically removed, memory_block_get() inside
walk_memory_blocks() will return NULL and skip try_reonline_memory_block().
Because the callback is skipped, the tmp pointer will not be advanced for
those blocks.

When the rollback loop moves to the next range, it will read from the beginning
of tmp, incorrectly applying the saved states of the first range to the blocks
of the second range. Can this desynchronization be avoided?

> +	unlock_device_hotplug();
> +
> +	kfree(online_types);
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(offline_and_remove_memory_ranges);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605211911.2160954-1-gourry@gourry.net?part=5