Re: [PATCH v6 09/10] dax/kmem: add sysfs interface for atomic whole-device hotplug

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing error cleanup in state_store() leaks resource reservations and corrupts state for subsequent unplug operations if dax_kmem_do_hotplug() fails.
--

commit 48ff7b34b13219e34e3823e263b700c3183a5d86
Author: Gregory Price <gourry@gourry.net>

dax/kmem: add sysfs interface for atomic whole-device hotplug

Adds a new 'state' sysfs attribute to daxX.Y devices that allows an atomic
whole-device hotplug operation across its entire memory region. The device
can be transitioned between unplugged, online, online_kernel, and
online_movable states.

> diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c
> index 72dcccee41e16..19effe0da3dce 100644
> --- a/drivers/dax/kmem.c
> +++ b/drivers/dax/kmem.c
[ ... ]
> @@ -123,14 +145,14 @@ static int dax_kmem_do_hotplug(struct dev_dax *dev_dax,
>  				kfree(data->res[i]);
>  				data->res[i] = NULL;
>  			}
> -			if (onlined)
> +			if (added)
>  				continue;
>  			return rc;

[Severity: High]
If this fails on the first range (added == 0), it returns immediately.
Since dax_kmem_init_resources() reserves all ranges beforehand, doesn't this
leave reservations for subsequent ranges (i+1 to N) allocated in data->res?

>  		}
> -		onlined++;
> +		added++;
>  	}
>  
> -	return onlined;
> +	return added;
>  }
[ ... ]
> @@ -256,6 +309,85 @@ static void dax_kmem_cleanup_resources(struct dev_dax *dev_dax,
[ ... ]
> +static ssize_t state_store(struct device *dev, struct device_attribute *attr,
> +			     const char *buf, size_t len)
> +{
[ ... ]
> +	/* Re-acquire resources if previously unplugged, otherwise no-op */
> +	rc = dax_kmem_init_resources(dev_dax, data);
> +	if (rc < 0)
> +		return rc;
> +
> +	rc = dax_kmem_do_hotplug(dev_dax, data, online_type);
> +	if (rc < 0)
> +		return rc;

[Severity: High]
If dax_kmem_do_hotplug() returns an error here, state_store() returns
directly without calling dax_kmem_cleanup_resources().

If the first range fails to hotplug, does this leave the remaining ranges
reserved but not added?

If the user later writes "unplugged" to sysfs, would dax_kmem_do_hotremove()
attempt to offline and remove memory that was never successfully added to the
hotplug subsystem?

> +
> +	data->state = online_type;
> +	return len;
> +}

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