timekeeping: Fix resource leak in tk_aux_sysfs_init() error paths

PATCH Superseded Malaya Kumar Rout 3 participants 6 messages Nov 10, 2025 – Nov 20, 2025 Timekeeping, clocksource core, NTP, alarmtimer

lore

Share & tools

lore

Malaya Kumar Rout

author

2025-11-10 07:01 UTC #0 lore raw

PATCH 1/1 timekeeping: Fix resource leak in tk_aux_sysfs_init() error paths

The tk_aux_sysfs_init() function returns immediately on error during the auxiliary clock initialization loop without cleaning up previously allocated kobjects and sysfs groups.

If kobject_create_and_add() or sysfs_create_group() fails during loop iteration, the parent kobjects (tko and auxo) and any previously created child kobjects are leaked.

Fix this by adding proper error handling with goto labels to ensure all allocated resources are cleaned up on failure. kobject_put() on the parent kobjects will handle cleanup of their children.

Signed-off-by Malaya Kumar Rout <[email protected]>

---
 kernel/time/timekeeping.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

kernel/time/timekeeping.c +15 -7

index 3a4d3b2e3f74..79019b25f188 100644
@@ -3060,29 +3060,37 @@ static const struct attribute_group aux_clock_enable_attr_group = {
 static int __init tk_aux_sysfs_init(void)
 {
 	struct kobject *auxo, *tko = kobject_create_and_add("time", kernel_kobj);
+	int ret;
 
 	if (!tko)
 		return -ENOMEM;
 
 	auxo = kobject_create_and_add("aux_clocks", tko);
 	if (!auxo) {
-		kobject_put(tko);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err_put_tko;
 	}
 
 	for (int i = 0; i < MAX_AUX_CLOCKS; i++) {
 		char id[2] = { [0] = '0' + i, };
 		struct kobject *clk = kobject_create_and_add(id, auxo);
 
-		if (!clk)
-			return -ENOMEM;
-
-		int ret = sysfs_create_group(clk, &aux_clock_enable_attr_group);
+		if (!clk) {
+			ret = -ENOMEM;
+			goto err_put_auxo;
+		}
 
+		ret = sysfs_create_group(clk, &aux_clock_enable_attr_group);
 		if (ret)
-			return ret;
+			goto err_put_auxo;
 	}
 	return 0;
+
+err_put_auxo:
+	kobject_put(auxo);
+err_put_tko:
+	kobject_put(tko);
+	return ret;
 }
 late_initcall(tk_aux_sysfs_init);

Signature · 1 line

2.51.0

Thomas Gleixner

2025-11-14 18:41 UTC #1 lore raw

Quoted 5 lines · Malaya Kumar Rout

kernel/time/timekeeping.c +1 -0

@@ -3060,29 +3060,37 @@ static const struct attribute_group aux_clock_enable_attr_group = {
 static int __init tk_aux_sysfs_init(void)
 {
 	struct kobject *auxo, *tko = kobject_create_and_add("time", kernel_kobj);
+	int ret;

kernel/time/timekeeping.c static const struct attribute_group aux_clock_enable_attr_group = { · line 3071

Quoted 10 lines

kernel/time/timekeeping.c +2 -2

 
 	if (!tko)
 		return -ENOMEM;
 
 	auxo = kobject_create_and_add("aux_clocks", tko);
 	if (!auxo) {
-		kobject_put(tko);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err_put_tko;

This ret variable is completely pointless as it is set to -ENOMEM in every error path. Just make the error path do 'return -ENOMEM;', no?

Quoted 17 lines

kernel/time/timekeeping.c +5 -4

 	}
 
 	for (int i = 0; i < MAX_AUX_CLOCKS; i++) {
 		char id[2] = { [0] = '0' + i, };
 		struct kobject *clk = kobject_create_and_add(id, auxo);
 
-		if (!clk)
-			return -ENOMEM;
-
-		int ret = sysfs_create_group(clk, &aux_clock_enable_attr_group);
+		if (!clk) {
+			ret = -ENOMEM;
+			goto err_put_auxo;
+		}
 
+		ret = sysfs_create_group(clk, &aux_clock_enable_attr_group);
 		if (ret)

kernel/time/timekeeping.c static const struct attribute_group aux_clock_enable_attr_group = { · line 3093

Quoted 10 lines

kernel/time/timekeeping.c +7 -1

-			return ret;
+			goto err_put_auxo;
 	}
 	return 0;
+
+err_put_auxo:
+	kobject_put(auxo);
+err_put_tko:
+	kobject_put(tko);
+	return ret;

You can simplify that with _one_ error label:

err:

	kobject_put(auxo);
	kobject_put(tko);
	return -ENOMEM;

because kobject_put() is NULL pointer safe.

Thanks,

tglx

Malaya Kumar Rout

author

2025-11-16 15:49 UTC #2 lore raw

Quoted 11 lines · Thomas Gleixner

Quoted 9 lines · Malaya Kumar Rout

kernel/time/timekeeping.c +1 -0

@@ -3060,29 +3060,37 @@ static const struct attribute_group aux_clock_enable_attr_group = {
 static int __init tk_aux_sysfs_init(void)
 {
      struct kobject *auxo, *tko = kobject_create_and_add("time", kernel_kobj);
+     int ret;
 
      if (!tko)
              return -ENOMEM;

Quoted 10 lines

kernel/time/timekeeping.c static const struct attribute_group aux_clock_enable_attr_group = { · line 3071
Quoted 6 lines
kernel/time/timekeeping.c +2 -2
      auxo = kobject_create_and_add("aux_clocks", tko);
      if (!auxo) {
-             kobject_put(tko);
-             return -ENOMEM;
+             ret = -ENOMEM;
+             goto err_put_tko;
This ret variable is completely pointless as it is set to -ENOMEM in every error path. Just make the error path do 'return -ENOMEM;', no?

While it's true that most error paths in this function return -ENOMEM, the sysfs_create_group() call can return different error codes depending on the failure mode:
- -ENOMEM: Memory allocation failure
- -EEXIST: Attribute group already exists
- -EINVAL: Invalid arguments
By preserving the 'ret' variable, we ensure that the actual error code from sysfs_create_group() is propagated to the caller. This provides more accurate error information for debugging and allows the caller to handle different error conditions appropriately.

Quoted 30 lines

Quoted 17 lines

kernel/time/timekeeping.c +5 -4

      }
 
      for (int i = 0; i < MAX_AUX_CLOCKS; i++) {
              char id[2] = { [0] = '0' + i, };
              struct kobject *clk = kobject_create_and_add(id, auxo);
 
-             if (!clk)
-                     return -ENOMEM;
-
-             int ret = sysfs_create_group(clk, &aux_clock_enable_attr_group);
+             if (!clk) {
+                     ret = -ENOMEM;
+                     goto err_put_auxo;
+             }
 
+             ret = sysfs_create_group(clk, &aux_clock_enable_attr_group);
              if (ret)

kernel/time/timekeeping.c static const struct attribute_group aux_clock_enable_attr_group = { · line 3093

Quoted 10 lines

kernel/time/timekeeping.c +7 -1

-                     return ret;
+                     goto err_put_auxo;
      }
      return 0;
+
+err_put_auxo:
+     kobject_put(auxo);
+err_put_tko:
+     kobject_put(tko);
+     return ret;

You can simplify that with _one_ error label:

Quoted 6 lines

err:
        kobject_put(auxo);
        kobject_put(tko);
        return -ENOMEM;
because kobject_put() is NULL pointer safe.

I agree with your suggestion to use a single error label since kobject_put() is NULL-safe.

Quoted 5 lines

Thanks,
tglx

Thank you for reviewing the patch and providing valuable feedback. I will incorporate your suggestion for the single error label in the next version while retaining the 'ret' variable for proper error propagation.

Thomas Gleixner

2025-11-16 17:14 UTC #3 lore raw

Quoted 11 lines · Malaya Kumar Rout

Quoted 10 lines · Thomas Gleixner
kernel/time/timekeeping.c static const struct attribute_group aux_clock_enable_attr_group = { · line 3071
Quoted 6 lines
      auxo = kobject_create_and_add("aux_clocks", tko);
      if (!auxo) {
kernel/time/timekeeping.c +2 -2
-             kobject_put(tko);
-             return -ENOMEM;
+             ret = -ENOMEM;
+             goto err_put_tko;
This ret variable is completely pointless as it is set to -ENOMEM in every error path. Just make the error path do 'return -ENOMEM;', no?

Quoted 10 lines

While it's true that most error paths in this function return -ENOMEM, the sysfs_create_group() call can return different error codes depending on the failure mode:
- -ENOMEM: Memory allocation failure
- -EEXIST: Attribute group already exists
- -EINVAL: Invalid arguments
By preserving the 'ret' variable, we ensure that the actual error code from sysfs_create_group() is propagated to the caller. This provides more accurate error information for debugging and allows the caller to handle different error conditions appropriately.

Fair enough

Quoted 35 lines

Quoted 17 lines

      }

      for (int i = 0; i < MAX_AUX_CLOCKS; i++) {
              char id[2] = { [0] = '0' + i, };
              struct kobject *clk = kobject_create_and_add(id, auxo);

kernel/time/timekeeping.c +5 -4

-             if (!clk)
-                     return -ENOMEM;
-
-             int ret = sysfs_create_group(clk, &aux_clock_enable_attr_group);
+             if (!clk) {
+                     ret = -ENOMEM;
+                     goto err_put_auxo;
+             }
 
+             ret = sysfs_create_group(clk, &aux_clock_enable_attr_group);
              if (ret)

kernel/time/timekeeping.c static const struct attribute_group aux_clock_enable_attr_group = { · line 3093

Quoted 10 lines

kernel/time/timekeeping.c +7 -1

-                     return ret;
+                     goto err_put_auxo;
      }
      return 0;
+
+err_put_auxo:
+     kobject_put(auxo);
+err_put_tko:
+     kobject_put(tko);
+     return ret;

You can simplify that with _one_ error label:

err:

        kobject_put(auxo);
        kobject_put(tko);
        return -ENOMEM;

Quoted 8 lines

Quoted 1 line
because kobject_put() is NULL pointer safe.
I agree with your suggestion to use a single error label since kobject_put() is NULL-safe.
Thank you for reviewing the patch and providing valuable feedback. I will incorporate your suggestion for the single error label in the next version while retaining the 'ret' variable for proper error propagation.

To avoid this ENOMEM nonsense you can split the stuff and do:

static int __init tk_aux_sysfs_init(void)

{
	struct kobject *auxo, *tko = kobject_create_and_add("time", kernel_kobj);
        int ret = -ENOMEM;

	if (!tko)
		return -ENOMEM;

	auxo = kobject_create_and_add("aux_clocks", tko);
	if (auxo)
        	ret = __tk_aux_sysfs_init(auxo);

	if (ret) {
		kobject_put(auxo);
		kobject_put(tko);
        }
	return ret;
}

which spares all the extra 'ret = -ENOMEM;' completely.

Thanks,

tglx

Malaya Kumar Rout

author

2025-11-20 15:02 UTC #4 lore raw

PATCH v2 1/1 timekeeping: Fix resource leak in tk_aux_sysfs_init() error paths

The tk_aux_sysfs_init() function returns immediately on error during the auxiliary clock initialization loop without cleaning up previously allocated kobjects and sysfs groups.

If kobject_create_and_add() or sysfs_create_group() fails during loop iteration, the parent kobjects (tko and auxo) and any previously created child kobjects are leaked.

Fix this by adding proper error handling with goto labels to ensure all allocated resources are cleaned up on failure. kobject_put() on the parent kobjects will handle cleanup of their children.

Signed-off-by Malaya Kumar Rout <[email protected]>

---
 kernel/time/timekeeping.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

kernel/time/timekeeping.c +12 -9

index 3a4d3b2e3f74..08e0943b54da 100644
@@ -3060,29 +3060,32 @@ static const struct attribute_group aux_clock_enable_attr_group = {
 static int __init tk_aux_sysfs_init(void)
 {
 	struct kobject *auxo, *tko = kobject_create_and_add("time", kernel_kobj);
+	int ret = -ENOMEM;
 
 	if (!tko)
-		return -ENOMEM;
+		return ret;
 
 	auxo = kobject_create_and_add("aux_clocks", tko);
-	if (!auxo) {
-		kobject_put(tko);
-		return -ENOMEM;
-	}
+	if (!auxo)
+		goto err_clean;
 
 	for (int i = 0; i < MAX_AUX_CLOCKS; i++) {
 		char id[2] = { [0] = '0' + i, };
 		struct kobject *clk = kobject_create_and_add(id, auxo);
 
 		if (!clk)
-			return -ENOMEM;
-
-		int ret = sysfs_create_group(clk, &aux_clock_enable_attr_group);
+			goto err_clean;
 
+		ret = sysfs_create_group(clk, &aux_clock_enable_attr_group);
 		if (ret)
-			return ret;
+			goto err_clean;
 	}
 	return 0;
+
+err_clean:
+	kobject_put(auxo);
+	kobject_put(tko);
+	return ret;
 }
 late_initcall(tk_aux_sysfs_init);

Signature · 1 line

2.51.0

tip-bot2 for Malaya Kumar Rout

merge bot Applied

2025-11-20 15:44 UTC #5 lore raw

The following commit has been merged into the timers/urgent branch of tip:

Commit-ID:     7b5ab04f035f829ed6008e4685501ec00b3e73c9
Gitweb:        https://git.kernel.org/tip/7b5ab04f035f829ed6008e4685501ec00b3e73c9
Author:        Malaya Kumar Rout <[email protected]>
AuthorDate:    Thu, 20 Nov 2025 20:32:13 +05:30
Committer:     Thomas Gleixner <[email protected]>
CommitterDate: Thu, 20 Nov 2025 16:40:48 +01:00

timekeeping: Fix resource leak in tk_aux_sysfs_init() error paths

tk_aux_sysfs_init() returns immediately on error during the auxiliary clock initialization loop without cleaning up previously allocated kobjects and sysfs groups.

If kobject_create_and_add() or sysfs_create_group() fails during loop iteration, the parent kobjects (tko and auxo) and any previously created child kobjects are leaked.

Fix this by adding proper error handling with goto labels to ensure all allocated resources are cleaned up on failure. kobject_put() on the parent kobjects will handle cleanup of their children.

Fixes 7b95663a3d96 ("timekeeping: Provide interface to control auxiliary clocks")

Signed-off-by Malaya Kumar Rout <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

Link https://patch.msgid.link/[email protected] open here

---
 kernel/time/timekeeping.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

kernel/time/timekeeping.c +12 -9

index 3a4d3b2..08e0943 100644
@@ -3060,29 +3060,32 @@ static const struct attribute_group aux_clock_enable_attr_group = {
 static int __init tk_aux_sysfs_init(void)
 {
 	struct kobject *auxo, *tko = kobject_create_and_add("time", kernel_kobj);
+	int ret = -ENOMEM;
 
 	if (!tko)
-		return -ENOMEM;
+		return ret;
 
 	auxo = kobject_create_and_add("aux_clocks", tko);
-	if (!auxo) {
-		kobject_put(tko);
-		return -ENOMEM;
-	}
+	if (!auxo)
+		goto err_clean;
 
 	for (int i = 0; i < MAX_AUX_CLOCKS; i++) {
 		char id[2] = { [0] = '0' + i, };
 		struct kobject *clk = kobject_create_and_add(id, auxo);
 
 		if (!clk)
-			return -ENOMEM;
-
-		int ret = sysfs_create_group(clk, &aux_clock_enable_attr_group);
+			goto err_clean;
 
+		ret = sysfs_create_group(clk, &aux_clock_enable_attr_group);
 		if (ret)
-			return ret;
+			goto err_clean;
 	}
 	return 0;
+
+err_clean:
+	kobject_put(auxo);
+	kobject_put(tko);
+	return ret;
 }
 late_initcall(tk_aux_sysfs_init);