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
