linux-kernel - Re: [PATCH] timekeeping: Fix resource leak in tk_aux_sysfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADD9qegc7VsPFnWgdhRQU-gSHnRtcU=M3+aQBjx5=6FB0FGv8w@mail.gmail.com>
Date: Sun, 16 Nov 2025 21:19:21 +0530
From: Malaya Kumar Rout <mrout@...hat.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: linux-kernel@...r.kernel.org, lyude@...hat.com, malayarout91@...il.com, 
	John Stultz <jstultz@...gle.com>, Stephen Boyd <sboyd@...nel.org>
Subject: Re: [PATCH] timekeeping: Fix resource leak in tk_aux_sysfs_init()
 error paths

On Sat, Nov 15, 2025 at 12:11 AM Thomas Gleixner <tglx@...utronix.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.