Re: [PATCH v4 7/9] dax/kmem: extract hotplug/hotremove helper functions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dax_kmem_do_hotplug() unconditionally attempts to add memory for all ranges, missing a check for successfully reserved I/O resources, leading to unsafe hotplug and a permanent memory leak on removal.
--

commit 9aae29a4ee0b2d84bab7e3e062cb31c70f75bf67
Author: Gregory Price <gourry@gourry.net>

dax/kmem: extract hotplug/hotremove helper functions

Refactor kmem _probe() _remove() by extracting init, cleanup, hotplug,
and hot-remove logic into separate helper functions.

> diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c
> index 41ccb618a1464..5bf36ab73f867 100644
> --- a/drivers/dax/kmem.c
> +++ b/drivers/dax/kmem.c
[ ... ]
> @@ -63,14 +63,195 @@ static void kmem_put_memory_types(void)
>  	mt_put_memory_types(&kmem_memory_types);
>  }
>  
> +/**
> + * dax_kmem_do_hotplug - hotplug memory for dax kmem device
[ ... ]
> +static int dax_kmem_do_hotplug(struct dev_dax *dev_dax,
> +			       struct dax_kmem_data *data,
> +			       int online_type)
> +{
> +	struct device *dev = &dev_dax->dev;
> +	int i, rc, onlined = 0;
> +	mhp_t mhp_flags;
> +
> +	for (i = 0; i < dev_dax->nr_range; i++) {
> +		struct range range;
> +
> +		rc = dax_kmem_range(dev_dax, i, &range);
> +		if (rc)
> +			continue;
> +
> +		mhp_flags = MHP_NID_IS_MGID;
> +		if (dev_dax->memmap_on_memory)
> +			mhp_flags |= MHP_MEMMAP_ON_MEMORY;
> +
> +		/*
> +		 * Ensure that future kexec'd kernels will not treat
> +		 * this as RAM automatically.
> +		 */
> +		rc = __add_memory_driver_managed(data->mgid, range.start,
> +				range_len(&range), kmem_name, mhp_flags,
> +				online_type);

[Severity: High]
Should this loop verify if the resource was actually reserved before
adding the memory?

In dax_kmem_init_resources(), if request_mem_region() fails for a range
but a previous range succeeded (mapped > 0), the failure is bypassed and
data->res[i] is left as NULL.

If we don't check for !data->res[i] here, does this unconditionally call
__add_memory_driver_managed() and hot-add memory for physical ranges that
failed reservation?

> +
> +		if (rc) {
> +			dev_warn(dev, "mapping%d: %#llx-%#llx memory add failed\n",
[ ... ]
> +static int dax_kmem_init_resources(struct dev_dax *dev_dax,
> +				   struct dax_kmem_data *data)
> +{
[ ... ]
> +		/* Region is permanently reserved if hotremove fails. */
> +		res = request_mem_region(range.start, range_len(&range),
> +					 data->res_name);
> +		if (!res) {
> +			dev_warn(dev, "mapping%d: %#llx-%#llx could not reserve region\n",
> +				 i, range.start, range.end);
> +			/*
> +			 * Once some memory has been onlined we can't
> +			 * assume that it can be un-onlined safely.
> +			 */
> +			if (mapped)
> +				continue;
> +			return -EBUSY;
> +		}
> +		data->res[i] = res;

[ ... ]
> +#ifdef CONFIG_MEMORY_HOTREMOVE
> +/**
> + * dax_kmem_do_hotremove - hot-remove memory for dax kmem device
[ ... ]
> +static int dax_kmem_do_hotremove(struct dev_dax *dev_dax,
> +				 struct dax_kmem_data *data)
> +{
> +	struct device *dev = &dev_dax->dev;
> +	int i, success = 0;
> +
> +	for (i = 0; i < dev_dax->nr_range; i++) {
> +		struct range range;
> +		int rc;
> +
> +		rc = dax_kmem_range(dev_dax, i, &range);
> +		if (rc)
> +			continue;
> +
> +		/* range was never added during probe, count as removed */
> +		if (!data->res[i]) {
> +			success++;
> +			continue;
> +		}

[Severity: High]
If dax_kmem_do_hotplug() erroneously added memory for a range with a
NULL data->res[i], will this check cause us to skip remove_memory()
for that range?

Does this strand the memory block online permanently?

> +
> +		rc = remove_memory(range.start, range_len(&range));

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