From: Kaushlendra Kumar kaushlendra.kumar@intel.com
[ Upstream commit 4b93d211bbffd3dce76664d95f2306d23e7215ce ]
Add proper error handling and resource cleanup to prevent memory leaks in add_boot_memory_ranges(). The function now checks for NULL return from kobject_create_and_add(), uses local buffer for range names to avoid dynamic allocation, and implements a cleanup path that removes previously created sysfs groups and kobjects on failure.
This prevents resource leaks when kobject creation or sysfs group creation fails during boot memory range initialization.
Signed-off-by: Kaushlendra Kumar kaushlendra.kumar@intel.com Reviewed-by: Tony Luck tony.luck@intel.com Link: https://patch.msgid.link/20251030023228.3956296-1-kaushlendra.kumar@intel.co... Signed-off-by: Rafael J. Wysocki rafael.j.wysocki@intel.com Signed-off-by: Sasha Levin sashal@kernel.org ---
LLM Generated explanations, may be completely bogus:
The following analysis evaluates whether the commit "ACPI: MRRM: Fix memory leaks and improve error handling" should be backported to stable kernel trees.
### 1. Comprehensive Analysis
**A. Problem Description** The commit addresses multiple defects in the `add_boot_memory_ranges()` function within `drivers/acpi/acpi_mrrm.c`: 1. **Memory Leak (Critical):** The original code used `name = kasprintf(GFP_KERNEL, "range%d", i)` inside a loop to generate kobject names. `kobject_create_and_add()` makes its own copy of the name. The original pointer `name` was never freed, leading to a memory leak for every memory range entry every time the system boots. 2. **Missing Error Checks:** - The code failed to check if the parent kobject `pkobj` was successfully created (NULL check missing). - The code failed to check if the child kobject `kobj` was successfully created before passing it to `sysfs_create_groups()`. If `kobj` were NULL (e.g., OOM), `sysfs_create_groups()` would dereference it, causing a crash. 3. **Incomplete Cleanup:** If an error occurred during the loop (e.g., one range failed to create), the function returned an error code but left previously created kobjects and sysfs groups valid and attached, leading to a partially initialized and potentially unstable state.
**B. Code Research & Fix Verification** - **Leak Fix:** The commit replaces `kasprintf` (dynamic allocation) with `char name[16]` and `scnprintf` (stack allocation). The format string is "range%d". Since `range` is 5 bytes and `%d` (integer) is at most 10 digits + sign, 16 bytes is sufficient to hold the string without overflow. This completely eliminates the memory leak. - **Robustness:** It adds explicit checks (`if (!pkobj)`, `if (!kobj)`) which prevents potential NULL pointer dereferences. - **Cleanup:** It allocates a temporary array `kobjs` to track created objects. If any step fails, the `cleanup` label iterates through this array to unwind the changes (`sysfs_remove_groups` and `kobject_put`), ensuring "all-or-nothing" semantics.
**C. Context & History** - The affected file `drivers/acpi/acpi_mrrm.c` was introduced in kernel v6.16 (commit `b9020bdb9f76`). - The buggy function `add_boot_memory_ranges` was introduced shortly after in the same cycle. - Since the current HEAD is `stable/linux-6.17.y`, this code is present and buggy in the current stable tree (6.17.y) and the previous one (6.16.y).
### 2. Stable Kernel Rules Assessment
- **Obviously Correct and Tested:** **Yes.** The fix uses standard kernel APIs (`kcalloc`, `kfree`, `kobject_put`) and standard error handling patterns. The stack buffer size is safe. - **Fixes a Real Bug:** **Yes.** It fixes a guaranteed memory leak on boot and prevents crashes under low-memory conditions. - **Important Issue:** **Yes.** While the leak size per boot is small (number of memory ranges), memory leaks are considered defects that should be fixed in stable, especially when the fix is low-risk. The crash prevention (NULL dereference) is also significant. - **Small and Contained:** **Yes.** The changes are limited to one function `add_boot_memory_ranges` in one file. - **No New Features/APIs:** **Yes.** The logic remains the same; only the implementation implementation is made correct and robust.
### 3. Risk vs. Benefit
- **Benefit:** Stops memory leaks on supported hardware. Prevents potential kernel oops during boot if memory is constrained. Ensures clean failure state. - **Risk:** **Low.** The new cleanup logic is straightforward. The array allocation `kcalloc` is checked. The logic handles the `acpi_mrrm` subsystem which is a specific driver, limiting the blast radius of any regression to just this ACPI feature.
### 4. Conclusion
This commit is a textbook stable backport candidate. It fixes a clear implementation flaw (allocated memory never freed) and missing error checks in code that exists in currently supported stable trees. The fix is safe, correct, and improves kernel reliability.
**YES**
drivers/acpi/acpi_mrrm.c | 43 ++++++++++++++++++++++++++++++---------- 1 file changed, 33 insertions(+), 10 deletions(-)
diff --git a/drivers/acpi/acpi_mrrm.c b/drivers/acpi/acpi_mrrm.c index a6dbf623e5571..6d69554c940ed 100644 --- a/drivers/acpi/acpi_mrrm.c +++ b/drivers/acpi/acpi_mrrm.c @@ -152,26 +152,49 @@ ATTRIBUTE_GROUPS(memory_range);
static __init int add_boot_memory_ranges(void) { - struct kobject *pkobj, *kobj; + struct kobject *pkobj, *kobj, **kobjs; int ret = -EINVAL; - char *name; + char name[16]; + int i;
pkobj = kobject_create_and_add("memory_ranges", acpi_kobj); + if (!pkobj) + return -ENOMEM;
- for (int i = 0; i < mrrm_mem_entry_num; i++) { - name = kasprintf(GFP_KERNEL, "range%d", i); - if (!name) { - ret = -ENOMEM; - break; - } + kobjs = kcalloc(mrrm_mem_entry_num, sizeof(*kobjs), GFP_KERNEL); + if (!kobjs) { + kobject_put(pkobj); + return -ENOMEM; + }
+ for (i = 0; i < mrrm_mem_entry_num; i++) { + scnprintf(name, sizeof(name), "range%d", i); kobj = kobject_create_and_add(name, pkobj); + if (!kobj) { + ret = -ENOMEM; + goto cleanup; + }
ret = sysfs_create_groups(kobj, memory_range_groups); - if (ret) - return ret; + if (ret) { + kobject_put(kobj); + goto cleanup; + } + kobjs[i] = kobj; }
+ kfree(kobjs); + return 0; + +cleanup: + for (int j = 0; j < i; j++) { + if (kobjs[j]) { + sysfs_remove_groups(kobjs[j], memory_range_groups); + kobject_put(kobjs[j]); + } + } + kfree(kobjs); + kobject_put(pkobj); return ret; }