Re: [PATCH v5 5/9] mm/memory_hotplug: offline_and_remove_memory_ranges()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Failure in Phase 2 causes partial memory unplug without structural rollback, leading to permanently unmapped kernel memory and array pointer desynchronization.
--

commit e98e55bd875fd49e00562bd6591c9e44420c4e52
Author: Gregory Price <gourry@gourry.net>

mm/memory_hotplug: offline_and_remove_memory_ranges()

This commit adds offline_and_remove_memory_ranges() to offline and remove
multiple memory ranges atomically under a single lock_device_hotplug(). It
splits the operation into a phase that offlines all ranges, followed by a
phase that removes them, reverting the offline step if any failure occurs.

> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index a66346def504b..7d56e0c6ede0c 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c

[ ... ]

> @@ -2429,58 +2429,98 @@ static int try_reonline_memory_block(struct memory_block *mem, void *arg)

[ ... ]

> +int offline_and_remove_memory_ranges(const struct range *ranges, int nr_ranges)
> +{

[ ... ]

> +	/*
> +	 * Phase 2: Remove each range. This essentially cannot fail as we hold
> +	 * the hotplug lock . WARN if that assumption is ever broken.
> +	 */
>  	if (!rc) {
> -		rc = try_remove_memory(start, size);
> -		if (rc)
> -			pr_err("%s: Failed to remove memory: %d", __func__, rc);
> +		for (i = 0; i < nr_ranges; i++) {
> +			rc = try_remove_memory(ranges[i].start,
> +					       range_len(&ranges[i]));
> +			if (WARN_ON_ONCE(rc)) {
> +				pr_err("%s: Failed to remove memory: %d",
> +				       __func__, rc);
> +				break;
> +			}
> +		}
>  	}
>  
> -	/*
> -	 * Rollback what we did. While memory onlining might theoretically fail
> -	 * (nacked by a notifier), it barely ever happens.
> -	 */
> +	/* On fail: roll back. Blocks that were already offline are skipped */
>  	if (rc) {
>  		tmp = online_types;
> -		walk_memory_blocks(start, size, &tmp,
> -				   try_reonline_memory_block);
> +		for (i = 0; i < nr_ranges; i++)
> +			walk_memory_blocks(ranges[i].start,
> +					   range_len(&ranges[i]), &tmp,
> +					   try_reonline_memory_block);

[Severity: High]
What happens if try_remove_memory() fails for a subsequent range after
successfully removing an earlier one?

If rc is set and we break out of the phase 2 loop, we enter this rollback
path. For the ranges that were already successfully removed,
walk_memory_blocks() will skip their blocks because memory_block_get()
returns NULL:

drivers/base/memory.c:walk_memory_blocks() {
    ...
    for (block_id = start_block_id; block_id <= end_block_id; block_id++) {
        mem = memory_block_get(block_id);
        if (!mem)
            continue;

        ret = func(mem, arg);
    ...
}

Because try_reonline_memory_block() is not called for these skipped blocks,
the tmp pointer into online_types won't be advanced. Could this cause the
rollback to apply the saved states of the removed blocks to the remaining
valid blocks, corrupting their state?

>  	}
>  	unlock_device_hotplug();
>  
>  	kfree(online_types);
>  	return rc;
>  }

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