Re: [PATCH v4 8/9] dax/kmem: add sysfs interface for atomic hotplug

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unbinding the device while memory is online causes a use-after-free of `data->res_name` and incorrectly clears the node memory type.
- [High] `dax_kmem_do_hotplug()` unconditionally adds memory regions to the system even if their physical address reservations failed.
- [Low] Lockless access to `data->state` in `hotplug_show` can race with concurrent updates.
--

commit 3599d063a2c3d29c40b1495050182e1083f34ccd
Author: Gregory Price <gourry@gourry.net>

dax/kmem: add sysfs interface for atomic hotplug

The dax kmem driver currently onlines memory automatically during
probe using the system's default online policy but provides no way
to control or query the entire region state at runtime.

This commit adds a 'hotplug' sysfs attribute allowing userspace to
control and query the entire memory region state. Unplug operations are
now atomic across the whole device.

> diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c
> index 5bf36ab73f867..46ee06d9f56b3 100644
> --- a/drivers/dax/kmem.c
> +++ b/drivers/dax/kmem.c

[ ... ]

> @@ -63,23 +69,41 @@ static void kmem_put_memory_types(void)
>  	mt_put_memory_types(&kmem_memory_types);
>  }
>  
> +/* True for the online states a kmem dax device can hold. */
> +static bool dax_kmem_state_is_online(int state)
> +{
> +	return state == MMOP_ONLINE ||
> +	       state == MMOP_ONLINE_KERNEL ||
> +	       state == MMOP_ONLINE_MOVABLE;
> +}
> +
>  /**
> - * dax_kmem_do_hotplug - hotplug memory for dax kmem device
> + * dax_kmem_do_hotplug - add the dev_dax memory ranges as system memory
>   * @dev_dax: the dev_dax instance
>   * @data: the dax_kmem_data structure with resource tracking
> + * @online_type: MMOP_OFFLINE to add the blocks offline, otherwise the online
> + *		 state (MMOP_ONLINE, MMOP_ONLINE_KERNEL, MMOP_ONLINE_MOVABLE)
> + *		 to bring them online in.
>   *
> - * Hotplugs all ranges in the dev_dax region as system memory.
> + * Adds all ranges in the dev_dax region as system memory, onlining them in
> + * the requested zone unless @online_type is MMOP_OFFLINE.
>   *
> - * Returns the number of successfully mapped ranges, or negative error.
>   */
>  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;
> +	int i, rc, added = 0;
>  	mhp_t mhp_flags;
>  
> +	if (dax_kmem_state_is_online(data->state))
> +		return -EINVAL;
> +
> +	if (online_type < MMOP_OFFLINE || online_type > MMOP_ONLINE_MOVABLE)
> +		return -EINVAL;
> +
>  	for (i = 0; i < dev_dax->nr_range; i++) {
>  		struct range range;
>  
> @@ -88,10 +112,10 @@ static int dax_kmem_do_hotplug(struct dev_dax *dev_dax,
>  		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]
Does this code unconditionally add memory regions to the system even if their
physical address reservations failed?

If `request_mem_region()` fails in `dax_kmem_init_resources()`, it skips
reserving the region (leaving `data->res[i] = NULL`), but continues the loop
if previous ranges succeeded.

However, `dax_kmem_do_hotplug()` iterates over all ranges and blindly calls
`__add_memory_driver_managed()` without checking if `data->res[i]` is valid.
This attempts to forcibly online unreserved physical addresses as System RAM.

If these address ranges overlap with active MMIO or firmware regions, could
this lead to severe system stability issues and memory corruption?

[ ... ]

> +static ssize_t hotplug_show(struct device *dev,
> +			    struct device_attribute *attr, char *buf)
> +{
> +	struct dax_kmem_data *data = dev_get_drvdata(dev);
> +	const char *state_str;
> +
> +	if (!data)
> +		return -ENXIO;
> +
> +	switch (data->state) {

[Severity: Low]
Does this lockless read of `data->state` race with concurrent updates?

`hotplug_show()` reads `data->state` locklessly to return the current state
string to userspace. Concurrently, `hotplug_store()` modifies `data->state`
while holding `data->lock`. 

Should this be protected by the lock, or properly annotated with
`READ_ONCE()`/`WRITE_ONCE()` to prevent compiler tearing?

> +	case DAX_KMEM_UNPLUGGED:
> +		state_str = "unplugged";
> +		break;

[ ... ]

> @@ -345,23 +516,20 @@ 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 success;
>  	int node = dev_dax->target_node;
>  	struct device *dev = &dev_dax->dev;
>  	struct dax_kmem_data *data = dev_get_drvdata(dev);
>  
> +	device_remove_file(dev, &dev_attr_hotplug);
>  	/*
> -	 * We have one shot for removing memory, if some memory blocks were not
> -	 * offline prior to calling this function remove_memory() will fail, and
> -	 * there is no way to hotremove this memory until reboot because device
> -	 * unbind will succeed even if we return failure.
> +	 * Blocks added under the legacy offline policy are present but offline;
> +	 * remove them on unbind as the driver always has.  If removal fails,
> +	 * leak the resources rather than freeing state that still backs present
> +	 * memory.  Online memory is left alone (dax_kmem_cleanup_resources()
> +	 * warns and leaks it) since offlining it here could deadlock the unbind.
>  	 */
> -	success = dax_kmem_do_hotremove(dev_dax, data);
> -	if (success < dev_dax->nr_range) {
> -		dev_err(dev, "Hotplug regions stuck online until reboot\n");
> +	if (data->state == MMOP_OFFLINE && dax_kmem_do_hotremove(dev_dax, data))
>  		return;
> -	}
> -
>  	dax_kmem_cleanup_resources(dev_dax, data);
>  	memory_group_unregister(data->mgid);
>  	kfree(data->res_name);

[Severity: High]
Can unbinding the device while memory is online cause a use-after-free
of `data->res_name` and incorrectly clear the node memory type?

When `data->state != MMOP_OFFLINE`, `dev_dax_kmem_remove()` skips
`dax_kmem_do_hotremove()` and calls `dax_kmem_cleanup_resources()`. 

`dax_kmem_cleanup_resources()` intentionally warns and returns early to leave
the `struct resource` objects active to prevent unbind deadlocks.

However, `dev_dax_kmem_remove()` unconditionally proceeds to call
`kfree(data->res_name)` and `kfree(data)`. Since the leaked `struct resource`
objects' name fields still point to `data->res_name`, will any subsequent
read of `/proc/iomem` trigger a KASAN Use-After-Free?

Additionally, does the unconditional continuation improperly call
`clear_node_memory_type()`, corrupting the memtype association while the
memory blocks are still active?

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