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] [day] [month] [year] [list]

Message-ID: <87y0o599yd.ffs@tglx>
Date: Sun, 16 Nov 2025 18:14:02 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: Malaya Kumar Rout <mrout@...hat.com>
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 Sun, Nov 16 2025 at 21:19, Malaya Kumar Rout wrote:
> On Sat, Nov 15, 2025 at 12:11 AM Thomas Gleixner <tglx@...utronix.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