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 <[email protected]>
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/[email protected]?part=9
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.