[v1] timekeeping: Fix resource leak in tk_aux_sysfs_init() error paths

[PATCH] timekeeping: Fix resource leak in tk_aux_sysfs_init() error paths

Posted by Malaya Kumar Rout 1 month, 1 week ago

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 <mrout@redhat.com>
---
 kernel/time/timekeeping.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 3a4d3b2e3f74..79019b25f188 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -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);
 
-- 
2.51.0

Re: [PATCH] timekeeping: Fix resource leak in tk_aux_sysfs_init() error paths

Posted by Thomas Gleixner 1 month ago

On Mon, Nov 10 2025 at 12:30, Malaya Kumar Rout wrote:
> @@ -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;

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?

>  	}
>  
>  	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;

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

Re: [PATCH] timekeeping: Fix resource leak in tk_aux_sysfs_init() error paths

Posted by Malaya Kumar Rout 1 month ago

On Sat, Nov 15, 2025 at 12:11 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Mon, Nov 10 2025 at 12:30, Malaya Kumar Rout wrote:
> > @@ -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;
>
> 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.

> >       }
> >
> >       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;
>
> You can simplify that with _one_ error label:
>
> 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.
>
> 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.

Re: [PATCH] timekeeping: Fix resource leak in tk_aux_sysfs_init() error paths

Posted by Thomas Gleixner 1 month ago

On Sun, Nov 16 2025 at 21:19, Malaya Kumar Rout wrote:
> On Sat, Nov 15, 2025 at 12:11 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>> >       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.

Fair enough

>> >       }
>> >
>> >       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;
>>
>> You can simplify that with _one_ error label:
>>
>> 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.
>
> 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

[PATCH v2] timekeeping: Fix resource leak in tk_aux_sysfs_init() error paths

Posted by Malaya Kumar Rout 4 weeks, 1 day ago

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 <mrout@redhat.com>
---
 kernel/time/timekeeping.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 3a4d3b2e3f74..08e0943b54da 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -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);
 
-- 
2.51.0

[tip: timers/urgent] timekeeping: Fix resource leak in tk_aux_sysfs_init() error paths

Posted by tip-bot2 for Malaya Kumar Rout 4 weeks, 1 day ago

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 <mrout@redhat.com>
AuthorDate:    Thu, 20 Nov 2025 20:32:13 +05:30
Committer:     Thomas Gleixner <tglx@linutronix.de>
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 <mrout@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://patch.msgid.link/20251120150213.246777-1-mrout@redhat.com
---
 kernel/time/timekeeping.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 3a4d3b2..08e0943 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -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);