* [PATCH] ACPI: mrrm: Fix memory leaks and improve error handling @ 2025-10-07 10:22 Kaushlendra Kumar 2025-10-22 19:31 ` Rafael J. Wysocki 0 siblings, 1 reply; 7+ messages in thread From: Kaushlendra Kumar @ 2025-10-07 10:22 UTC (permalink / raw) To: rafael, lenb; +Cc: linux-acpi, Kaushlendra Kumar 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(), frees allocated names after use, 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> --- drivers/acpi/acpi_mrrm.c | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/drivers/acpi/acpi_mrrm.c b/drivers/acpi/acpi_mrrm.c index 47ea3ccc2142..6ec42eb48783 100644 --- a/drivers/acpi/acpi_mrrm.c +++ b/drivers/acpi/acpi_mrrm.c @@ -152,23 +152,48 @@ static __init int add_boot_memory_ranges(void) struct kobject *pkobj, *kobj; int ret = -EINVAL; char *name; + 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++) { + for (i = 0; i < mrrm_mem_entry_num; i++) { name = kasprintf(GFP_KERNEL, "range%d", i); if (!name) { ret = -ENOMEM; - break; + goto cleanup; } kobj = kobject_create_and_add(name, pkobj); + kfree(name); + 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; + } } + return 0; + +cleanup: + for (int j = 0; j < i; j++) { + char cleanup_name[32]; + struct kobject *cleanup_kobj; + + snprintf(cleanup_name, sizeof(cleanup_name), "range%d", j); + cleanup_kobj = kobject_get(pkobj); + if (cleanup_kobj) { + sysfs_remove_groups(cleanup_kobj, memory_range_groups); + kobject_put(cleanup_kobj); + } + } + kobject_put(pkobj); return ret; } -- 2.34.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] ACPI: mrrm: Fix memory leaks and improve error handling 2025-10-07 10:22 [PATCH] ACPI: mrrm: Fix memory leaks and improve error handling Kaushlendra Kumar @ 2025-10-22 19:31 ` Rafael J. Wysocki 2025-10-22 19:58 ` Luck, Tony 0 siblings, 1 reply; 7+ messages in thread From: Rafael J. Wysocki @ 2025-10-22 19:31 UTC (permalink / raw) To: Kaushlendra Kumar, Tony Luck; +Cc: lenb, linux-acpi On Tue, Oct 7, 2025 at 12:24 PM Kaushlendra Kumar <kaushlendra.kumar@intel.com> wrote: > > 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(), frees allocated names after use, 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> > --- > drivers/acpi/acpi_mrrm.c | 33 +++++++++++++++++++++++++++++---- > 1 file changed, 29 insertions(+), 4 deletions(-) > > diff --git a/drivers/acpi/acpi_mrrm.c b/drivers/acpi/acpi_mrrm.c > index 47ea3ccc2142..6ec42eb48783 100644 > --- a/drivers/acpi/acpi_mrrm.c > +++ b/drivers/acpi/acpi_mrrm.c > @@ -152,23 +152,48 @@ static __init int add_boot_memory_ranges(void) > struct kobject *pkobj, *kobj; > int ret = -EINVAL; > char *name; > + 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++) { Yes, i should be declared in the preamble. > + for (i = 0; i < mrrm_mem_entry_num; i++) { > name = kasprintf(GFP_KERNEL, "range%d", i); > if (!name) { > ret = -ENOMEM; > - break; > + goto cleanup; > } > > kobj = kobject_create_and_add(name, pkobj); > + kfree(name); OK, this fixes a memory leak. > + if (!kobj) { > + ret = -ENOMEM; > + goto cleanup; Why terminate this? Why not continue? > + } > > ret = sysfs_create_groups(kobj, memory_range_groups); > - if (ret) > - return ret; Well, this returns already, but I'm not sure why. Tony, wouldn't it be better to continue? > + if (ret) { > + kobject_put(kobj); > + goto cleanup; I would do a "continue" instead. > + } > } > > + return 0; > + > +cleanup: > + for (int j = 0; j < i; j++) { > + char cleanup_name[32]; > + struct kobject *cleanup_kobj; > + > + snprintf(cleanup_name, sizeof(cleanup_name), "range%d", j); > + cleanup_kobj = kobject_get(pkobj); > + if (cleanup_kobj) { > + sysfs_remove_groups(cleanup_kobj, memory_range_groups); > + kobject_put(cleanup_kobj); > + } > + } > + kobject_put(pkobj); > return ret; > } > > -- ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ACPI: mrrm: Fix memory leaks and improve error handling 2025-10-22 19:31 ` Rafael J. Wysocki @ 2025-10-22 19:58 ` Luck, Tony 2025-10-22 20:17 ` Rafael J. Wysocki 2025-10-28 4:48 ` Kumar, Kaushlendra 0 siblings, 2 replies; 7+ messages in thread From: Luck, Tony @ 2025-10-22 19:58 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: Kaushlendra Kumar, lenb, linux-acpi On Wed, Oct 22, 2025 at 09:31:29PM +0200, Rafael J. Wysocki wrote: > On Tue, Oct 7, 2025 at 12:24 PM Kaushlendra Kumar > <kaushlendra.kumar@intel.com> wrote: > > > > 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(), frees allocated names after use, 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> > > --- > > drivers/acpi/acpi_mrrm.c | 33 +++++++++++++++++++++++++++++---- > > 1 file changed, 29 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/acpi/acpi_mrrm.c b/drivers/acpi/acpi_mrrm.c > > index 47ea3ccc2142..6ec42eb48783 100644 > > --- a/drivers/acpi/acpi_mrrm.c > > +++ b/drivers/acpi/acpi_mrrm.c > > @@ -152,23 +152,48 @@ static __init int add_boot_memory_ranges(void) > > struct kobject *pkobj, *kobj; > > int ret = -EINVAL; > > char *name; > > + 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++) { > > Yes, i should be declared in the preamble. > > > + for (i = 0; i < mrrm_mem_entry_num; i++) { > > name = kasprintf(GFP_KERNEL, "range%d", i); > > if (!name) { > > ret = -ENOMEM; > > - break; > > + goto cleanup; > > } > > > > kobj = kobject_create_and_add(name, pkobj); > > + kfree(name); > > OK, this fixes a memory leak. I didn't realize that kobject_create_and_add() made its own copy of the "name" parameter. Maybe the code should avoid the alloc/free by making "name" a local variable? char name[16] = "rangeXXXXXXXXXX"; sprintf(&name[5], "%d", i); > > + if (!kobj) { > > + ret = -ENOMEM; > > + goto cleanup; > > Why terminate this? Why not continue? Terminating here (and below) will go to Kaushlendra's cleanup code to remove all the ranges. Maybe this is better than having some random subset of files appear (based on which allocations succeeded/failed)? > > + } > > > > ret = sysfs_create_groups(kobj, memory_range_groups); > > - if (ret) > > - return ret; > > Well, this returns already, but I'm not sure why. Tony, wouldn't it > be better to continue? > > > + if (ret) { > > + kobject_put(kobj); > > + goto cleanup; > > I would do a "continue" instead. > > > + } > > } > > > > + return 0; > > + > > +cleanup: > > + for (int j = 0; j < i; j++) { > > + char cleanup_name[32]; > > + struct kobject *cleanup_kobj; > > + > > + snprintf(cleanup_name, sizeof(cleanup_name), "range%d", j); > > + cleanup_kobj = kobject_get(pkobj); > > + if (cleanup_kobj) { > > + sysfs_remove_groups(cleanup_kobj, memory_range_groups); > > + kobject_put(cleanup_kobj); > > + } > > + } > > + kobject_put(pkobj); > > return ret; > > } > > > > -- -Tony ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ACPI: mrrm: Fix memory leaks and improve error handling 2025-10-22 19:58 ` Luck, Tony @ 2025-10-22 20:17 ` Rafael J. Wysocki 2025-10-28 4:48 ` Kumar, Kaushlendra 1 sibling, 0 replies; 7+ messages in thread From: Rafael J. Wysocki @ 2025-10-22 20:17 UTC (permalink / raw) To: Luck, Tony; +Cc: Rafael J. Wysocki, Kaushlendra Kumar, lenb, linux-acpi On Wed, Oct 22, 2025 at 9:58 PM Luck, Tony <tony.luck@intel.com> wrote: > > On Wed, Oct 22, 2025 at 09:31:29PM +0200, Rafael J. Wysocki wrote: > > On Tue, Oct 7, 2025 at 12:24 PM Kaushlendra Kumar > > <kaushlendra.kumar@intel.com> wrote: > > > > > > 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(), frees allocated names after use, 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> > > > --- > > > drivers/acpi/acpi_mrrm.c | 33 +++++++++++++++++++++++++++++---- > > > 1 file changed, 29 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/acpi/acpi_mrrm.c b/drivers/acpi/acpi_mrrm.c > > > index 47ea3ccc2142..6ec42eb48783 100644 > > > --- a/drivers/acpi/acpi_mrrm.c > > > +++ b/drivers/acpi/acpi_mrrm.c > > > @@ -152,23 +152,48 @@ static __init int add_boot_memory_ranges(void) > > > struct kobject *pkobj, *kobj; > > > int ret = -EINVAL; > > > char *name; > > > + 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++) { > > > > Yes, i should be declared in the preamble. > > > > > + for (i = 0; i < mrrm_mem_entry_num; i++) { > > > name = kasprintf(GFP_KERNEL, "range%d", i); > > > if (!name) { > > > ret = -ENOMEM; > > > - break; > > > + goto cleanup; > > > } > > > > > > kobj = kobject_create_and_add(name, pkobj); > > > + kfree(name); > > > > OK, this fixes a memory leak. > > I didn't realize that kobject_create_and_add() made its own copy of > the "name" parameter. Maybe the code should avoid the alloc/free by > making "name" a local variable? > > char name[16] = "rangeXXXXXXXXXX"; > > sprintf(&name[5], "%d", i); That'd work. > > > + if (!kobj) { > > > + ret = -ENOMEM; > > > + goto cleanup; > > > > Why terminate this? Why not continue? > > Terminating here (and below) will go to Kaushlendra's cleanup > code to remove all the ranges. Maybe this is better than having > some random subset of files appear (based on which allocations > succeeded/failed)? Well, I guess it can be argued both ways and either one is fine with me. If that's what you prefer, let it be done, but the changelog needs a bit of work because the cleanup is done in error cases. > > > + } > > > > > > ret = sysfs_create_groups(kobj, memory_range_groups); > > > - if (ret) > > > - return ret; > > > > Well, this returns already, but I'm not sure why. Tony, wouldn't it > > be better to continue? > > > > > + if (ret) { > > > + kobject_put(kobj); > > > + goto cleanup; > > > > I would do a "continue" instead. > > > > > + } > > > } > > > > > > + return 0; > > > + > > > +cleanup: > > > + for (int j = 0; j < i; j++) { > > > + char cleanup_name[32]; > > > + struct kobject *cleanup_kobj; > > > + > > > + snprintf(cleanup_name, sizeof(cleanup_name), "range%d", j); > > > + cleanup_kobj = kobject_get(pkobj); > > > + if (cleanup_kobj) { > > > + sysfs_remove_groups(cleanup_kobj, memory_range_groups); > > > + kobject_put(cleanup_kobj); > > > + } > > > + } > > > + kobject_put(pkobj); > > > return ret; > > > } > > > > > > -- > > -Tony ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] ACPI: mrrm: Fix memory leaks and improve error handling 2025-10-22 19:58 ` Luck, Tony 2025-10-22 20:17 ` Rafael J. Wysocki @ 2025-10-28 4:48 ` Kumar, Kaushlendra 2025-10-28 16:04 ` Luck, Tony 1 sibling, 1 reply; 7+ messages in thread From: Kumar, Kaushlendra @ 2025-10-28 4:48 UTC (permalink / raw) To: Luck, Tony, Rafael J. Wysocki; +Cc: lenb@kernel.org, linux-acpi@vger.kernel.org n Wed, Oct 22, 2025 at 09:31:29PM +0200, Rafael J. Wysocki wrote: > > + if (!kobj) { > > + ret = -ENOMEM; > > + goto cleanup; > > Why terminate this? Why not continue? > I didn't realize that kobject_create_and_add() made its own copy of the "name" parameter. Maybe the code should avoid the alloc/free by making "name" a local variable? > > char name[16] = "rangeXXXXXXXXXX"; > > sprintf(&name[5], "%d", i); > Terminating here (and below) will go to Kaushlendra's cleanup code to remove all the ranges. Maybe this is better than having some random subset of files appear (based on which allocations succeeded/failed)? Tony is exactly right here. I believe the current approach is more appropriate Here. Regarding Tony's suggestion about the local buffer - that's a valid optimization, but I'd prefer to keep the current approach for clarity. The kasprintf/kfree pattern is more explicit about memory management in kernel. The cleanup path ensures we maintain system consistency - either we successfully create the complete interface, or we leave the system in a clean state with no partial artifacts. Thanks for the detailed review! Kaushlendra ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] ACPI: mrrm: Fix memory leaks and improve error handling 2025-10-28 4:48 ` Kumar, Kaushlendra @ 2025-10-28 16:04 ` Luck, Tony 2025-10-28 16:17 ` Kumar, Kaushlendra 0 siblings, 1 reply; 7+ messages in thread From: Luck, Tony @ 2025-10-28 16:04 UTC (permalink / raw) To: Kumar, Kaushlendra, Rafael J. Wysocki Cc: lenb@kernel.org, linux-acpi@vger.kernel.org > n Wed, Oct 22, 2025 at 09:31:29PM +0200, Rafael J. Wysocki wrote: > > > + if (!kobj) { > > > + ret = -ENOMEM; > > > + goto cleanup; > > > > Why terminate this? Why not continue? > > > I didn't realize that kobject_create_and_add() made its own copy of the "name" parameter. Maybe the code should avoid the alloc/free by making "name" a local variable? > > > > char name[16] = "rangeXXXXXXXXXX"; > > > > sprintf(&name[5], "%d", i); > > > Terminating here (and below) will go to Kaushlendra's cleanup code to remove all the ranges. Maybe this is better than having some random subset of files appear (based on which allocations succeeded/failed)? > > Tony is exactly right here. I believe the current approach is more appropriate > Here. > > Regarding Tony's suggestion about the local buffer - that's a valid optimization, > but I'd prefer to keep the current approach for clarity. The kasprintf/kfree > pattern is more explicit about memory management in kernel. Why? I'll note that the cleanup code you added uses a local buffer "char cleanup_name[32];" to hold filenames (though now that I look back at the patch, that string isn't actually used!) > > The cleanup path ensures we maintain system consistency - either we successfully > create the complete interface, or we leave the system in a clean state with no > partial artifacts. > > Thanks for the detailed review! -Tony ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] ACPI: mrrm: Fix memory leaks and improve error handling 2025-10-28 16:04 ` Luck, Tony @ 2025-10-28 16:17 ` Kumar, Kaushlendra 0 siblings, 0 replies; 7+ messages in thread From: Kumar, Kaushlendra @ 2025-10-28 16:17 UTC (permalink / raw) To: Luck, Tony, Rafael J. Wysocki; +Cc: lenb@kernel.org, linux-acpi@vger.kernel.org On Wed, Oct 22, 2025 at 09:31:29PM +0200, Tony Luck wrote: > Why? I'll note that the cleanup code you added uses a local buffer "char cleanup_name[32];" > to hold filenames (though now that I look back at the patch, that string isn't actually used!) Got it. Looking at the cleanup code again, I see the inconsistency - I'm using a local buffer there but kasprintf() in the main loop. I will use a local buffer approach. Also thanks for catching the unused cleanup_name variable - I'll fix that in the cleanup logic as well. I'll send a v2 with these improvements. Thanks for the review! ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-10-28 16:17 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-10-07 10:22 [PATCH] ACPI: mrrm: Fix memory leaks and improve error handling Kaushlendra Kumar 2025-10-22 19:31 ` Rafael J. Wysocki 2025-10-22 19:58 ` Luck, Tony 2025-10-22 20:17 ` Rafael J. Wysocki 2025-10-28 4:48 ` Kumar, Kaushlendra 2025-10-28 16:04 ` Luck, Tony 2025-10-28 16:17 ` Kumar, Kaushlendra
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).