Re: [PATCH v7 08/10] 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:

Pre-existing issues:
- [High] If a DAX device has multiple ranges and at least one is too small to be aligned, removing the device will unconditionally leak driver data and memory group registrations.
--

--- Patch [8]: [PATCH v7 08/10] dax/kmem: extract hotplug/hotremove helper functions ---
commit 44ad562618b0448a09ed81ab105fa4c8551b6718
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. This splits the
hotplug and init phases to have resources available for hot-add, while
enabling future granular hotplug operation control.

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Gregory Price <gourry@gourry.net>

> diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c
> index 38ed5c4e9c83d..6174f7d3d05bd 100644
> --- a/drivers/dax/kmem.c
> +++ b/drivers/dax/kmem.c

[ ... ]

> +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;
> +		}
> +
> +		rc = remove_memory(range.start, range_len(&range));
> +		if (rc == 0) {
> +			/* Release the resource for the successfully removed range */
> +			remove_resource(data->res[i]);
> +			kfree(data->res[i]);
> +			data->res[i] = NULL;
> +			success++;
> +			continue;
> +		}
> +		any_hotremove_failed = true;
> +		dev_err(dev, "mapping%d: %#llx-%#llx hotremove failed\n",
> +			i, range.start, range.end);
> +	}
> +
> +	return success;
> +}

[ ... ]

> @@ -208,7 +354,7 @@ static int dev_dax_kmem_probe(struct dev_dax *dev_dax)
>  #ifdef CONFIG_MEMORY_HOTREMOVE
>  static void dev_dax_kmem_remove(struct dev_dax *dev_dax)
>  {
> -	int i, success = 0;
> +	int success;
>  	int node = dev_dax->target_node;
>  	struct device *dev = &dev_dax->dev;
>  	struct dax_kmem_data *data = dev_get_drvdata(dev);
> @@ -219,48 +365,25 @@ static void dev_dax_kmem_remove(struct dev_dax *dev_dax)
>  	 * there is no way to hotremove this memory until reboot because device
>  	 * unbind will succeed even if we return failure.
>  	 */
> -	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 */
> -		if (!data->res[i]) {
> -			success++;
> -			continue;
> -		}
> -
> -		rc = remove_memory(range.start, range_len(&range));
> -		if (rc == 0) {
> -			remove_resource(data->res[i]);
> -			kfree(data->res[i]);
> -			data->res[i] = NULL;
> -			success++;
> -			continue;
> -		}
> -		any_hotremove_failed = true;
> -		dev_err(dev,
> -			"mapping%d: %#llx-%#llx cannot be hotremoved until the next reboot\n",
> -				i, range.start, range.end);
> +	success = dax_kmem_do_hotremove(dev_dax, data);
> +	if (success < dev_dax->nr_range) {
> +		dev_err(dev, "Hotplug regions stuck online until reboot\n");
> +		return;
>  	}

[Severity: High]
This is a pre-existing issue, but does this logic handle unaligned ranges
correctly during device unbind?

If a DAX device is created with a memory range smaller than the block
alignment, dax_kmem_range() returns an error (like -ENOSPC) during probe,
but the device can still probe successfully.

During removal, dax_kmem_do_hotremove() will also get an error from
dax_kmem_range() for that small range and skip incrementing the success
counter.

This causes success to evaluate as strictly less than dev_dax->nr_range,
resulting in an early return here.

Since device unbind can be triggered from userspace, does this early return
unconditionally leak the memory group registrations, device name string,
and driver data on every unbind for devices with unaligned ranges?

>  
> -	if (success >= dev_dax->nr_range) {
> -		memory_group_unregister(data->mgid);
> -		kfree(data->res_name);
> -		kfree(data);
> -		dev_set_drvdata(dev, NULL);

[ ... ]

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