In livepatch_init(), if klp_enable_patch() fails, the previously
created kobject and sysfs file are never cleaned up, causing a
resource leak. Capture the return value and add proper cleanup
on the error path.

Signed-off-by: Rui Qi <qirui.001@bytedance.com>
---
 .../selftests/livepatch/test_modules/test_klp_syscall.c    | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c b/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c
index 0630ffd9d9a1..d631acae48b9 100644
--- a/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c
+++ b/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c
@@ -109,7 +109,12 @@ static int livepatch_init(void)
 	 */
 	npids = npids_pending;
 
-	return klp_enable_patch(&patch);
+	ret = klp_enable_patch(&patch);
+	if (ret) {
+		sysfs_remove_file(klp_kobj, &klp_attr.attr);
+		kobject_put(klp_kobj);
+	}
+	return ret;
 }
 
 static void livepatch_exit(void)
-- 
2.20.1

On Tue, 02 Jun 2026 20:45:09 +0800, Rui Qi <qirui.001@bytedance.com> wrote:
> diff --git a/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c b/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c
> index 0630ffd9d9a1..d631acae48b9 100644
> --- a/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c
> +++ b/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c
> @@ -109,7 +109,12 @@ static int livepatch_init(void)
>  	 */
>  	npids = npids_pending;
>  
> -	return klp_enable_patch(&patch);
> +	ret = klp_enable_patch(&patch);
> +	if (ret) {
> +		sysfs_remove_file(klp_kobj, &klp_attr.attr);
> +		kobject_put(klp_kobj);
> +	}
> +	return ret;

Is sysfs_remove_file() needed? I think that kobject_put() should remove
it automatically since the object is bound to sysfs.

-- 
Miroslav

On 6/3/26 9:09 PM, Miroslav Benes wrote:
> On Tue, 02 Jun 2026 20:45:09 +0800, Rui Qi <qirui.001@bytedance.com> wrote:
>> diff --git a/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c b/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c
>> index 0630ffd9d9a1..d631acae48b9 100644
>> --- a/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c
>> +++ b/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c
>> @@ -109,7 +109,12 @@ static int livepatch_init(void)
>>  	 */
>>  	npids = npids_pending;
>>  
>> -	return klp_enable_patch(&patch);
>> +	ret = klp_enable_patch(&patch);
>> +	if (ret) {
>> +		sysfs_remove_file(klp_kobj, &klp_attr.attr);
>> +		kobject_put(klp_kobj);
>> +	}
>> +	return ret;
> 
> Is sysfs_remove_file() needed? I think that kobject_put() should remove
> it automatically since the object is bound to sysfs.
> 

You are right, the sysfs_remove_file() call is redundant. When
kobject_put() drops the refcount to zero, kobject_cleanup() is
invoked, which calls __kobject_del() -> sysfs_remove_dir(). The
latter removes the entire kobject directory including all files
created under it via sysfs_create_file(). This is also consistent
with the livepatch_exit() cleanup path, which only calls
kobject_put() without an explicit sysfs_remove_file().

I will send v2 with sysfs_remove_file() removed.

Thank you for the review.

In livepatch_init(), if klp_enable_patch() fails, the previously
created kobject and sysfs file are never cleaned up, causing a
resource leak. Capture the return value and add proper cleanup
on the error path.

Signed-off-by: Rui Qi <qirui.001@bytedance.com>
---
Changes in v2:
- Remove sysfs_remove_file() from the error path as suggested by
  Miroslav Benes. kobject_put() alone is sufficient because it
  eventually calls kobject_cleanup() -> __kobject_del() ->
  sysfs_remove_dir(), which removes the entire directory including
  all files created under it.

 .../selftests/livepatch/test_modules/test_klp_syscall.c     | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c b/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c
index 0630ffd9d9a1..08aacc0e14de 100644
--- a/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c
+++ b/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c
@@ -109,7 +109,11 @@ static int livepatch_init(void)
 	 */
 	npids = npids_pending;
 
-	return klp_enable_patch(&patch);
+	ret = klp_enable_patch(&patch);
+	if (ret)
+		kobject_put(klp_kobj);
+
+	return ret;
 }
 
 static void livepatch_exit(void)
-- 
2.20.1

On 2026-06-04 16:32 +0800, Rui Qi wrote:
> In livepatch_init(), if klp_enable_patch() fails, the previously
> created kobject and sysfs file are never cleaned up, causing a
> resource leak. Capture the return value and add proper cleanup
> on the error path.
> 
> Signed-off-by: Rui Qi <qirui.001@bytedance.com>

Acked-by: Miroslav Benes <mbenes@suse.cz>

On Thu 2026-06-04 16:32:08, Rui Qi wrote:
> In livepatch_init(), if klp_enable_patch() fails, the previously
> created kobject and sysfs file are never cleaned up, causing a
> resource leak. Capture the return value and add proper cleanup
> on the error path.
> 
> Signed-off-by: Rui Qi <qirui.001@bytedance.com>

The patch looks good to me. I would ignore the proble reported by
Sashiko. so:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

PS: I am going to push this patch early next week if nobody complained
    in the meantime.

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Relying exclusively on kobject_put() to clean up sysfs attributes backed by module memory introduces a use-after-free window during module unload if CONFIG_DEBUG_KOBJECT_RELEASE is enabled.
--

commit 89541aa9ecd8e356eaf4884fa2b111bc605a7933
Author: Rui Qi <qirui.001@bytedance.com>

selftests/livepatch: fix resource leak in test_klp_syscall init error path

This commit attempts to fix a resource leak in livepatch_init() by calling
kobject_put() on the error path if klp_enable_patch() fails.

> --- a/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c
> +++ b/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c
[ ... ]
> @@ -98,7 +98,11 @@ static int livepatch_init(void)
>  	 */
>  	npids = npids_pending;
>  
> -	return klp_enable_patch(&patch);
> +	ret = klp_enable_patch(&patch);
> +	if (ret)
> +		kobject_put(klp_kobj);

[Severity: High]
This isn't a bug introduced by this patch, but it replicates a pre-existing
issue into the error path.

Does relying exclusively on kobject_put() here create a use-after-free window
during module unload?

If CONFIG_DEBUG_KOBJECT_RELEASE is enabled, kobject_put() drops the refcount
but schedules kobject_cleanup() (and thus sysfs_remove_dir()) to run
asynchronously via a workqueue after a 1 to 4 second delay. Because klp_attr
and its show callback are allocated in the module's memory, returning an error
here causes the module loader to free that memory immediately.

During this delay, the sysfs file remains fully accessible. If a user reads
this file, would sysfs_kf_seq_show() dereference the attribute and callback
that now point to unmapped module memory?

Should kobject_del(klp_kobj) be called prior to kobject_put() to
synchronously remove the sysfs entries?

> +
> +	return ret;
>  }
>  
>  static void livepatch_exit(void)
>  {
>  	kobject_put(klp_kobj);
>  }

[Severity: High]
This is a pre-existing issue, but the exact same vulnerability exists here in
the unmodified module exit path where kobject_put() is the only cleanup
mechanism before the module unloads.

Could we also add kobject_del(klp_kobj) before kobject_put() in
livepatch_exit() to ensure sysfs entries are synchronously removed before
module memory is freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604083208.1071428-1-qirui.001@bytedance.com?part=1

On Thu 2026-06-04 08:42:13, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Relying exclusively on kobject_put() to clean up sysfs attributes backed by module memory introduces a use-after-free window during module unload if CONFIG_DEBUG_KOBJECT_RELEASE is enabled.
> --
> 
> commit 89541aa9ecd8e356eaf4884fa2b111bc605a7933
> Author: Rui Qi <qirui.001@bytedance.com>
> 
> selftests/livepatch: fix resource leak in test_klp_syscall init error path
> 
> This commit attempts to fix a resource leak in livepatch_init() by calling
> kobject_put() on the error path if klp_enable_patch() fails.
> 
> > --- a/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c
> > +++ b/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c
> [ ... ]
> > @@ -98,7 +98,11 @@ static int livepatch_init(void)
> >  	 */
> >  	npids = npids_pending;
> >  
> > -	return klp_enable_patch(&patch);
> > +	ret = klp_enable_patch(&patch);
> > +	if (ret)
> > +		kobject_put(klp_kobj);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but it replicates a pre-existing
> issue into the error path.
> 
> Does relying exclusively on kobject_put() here create a use-after-free window
> during module unload?

Sigh, I always get lost when looking at kobject-related API.

> If CONFIG_DEBUG_KOBJECT_RELEASE is enabled, kobject_put() drops the refcount
> but schedules kobject_cleanup() (and thus sysfs_remove_dir()) to run
> asynchronously via a workqueue after a 1 to 4 second delay. Because klp_attr
> and its show callback are allocated in the module's memory, returning an error
> here causes the module loader to free that memory immediately.
>
> During this delay, the sysfs file remains fully accessible. If a user reads
> this file, would sysfs_kf_seq_show() dereference the attribute and callback
> that now point to unmapped module memory?

This sounds correct.

> Should kobject_del(klp_kobj) be called prior to kobject_put() to
> synchronously remove the sysfs entries?

I am not sure if this would be a complete solution. I do not see
anywhere that kobject_del() would be synchronous. I think that
the file still might stay opened.

> > +
> > +	return ret;
> >  }
> >  
> >  static void livepatch_exit(void)
> >  {
> >  	kobject_put(klp_kobj);
> >  }
> 
> [Severity: High]
> This is a pre-existing issue, but the exact same vulnerability exists here in
> the unmodified module exit path where kobject_put() is the only cleanup
> mechanism before the module unloads.
> 
> Could we also add kobject_del(klp_kobj) before kobject_put() in
> livepatch_exit() to ensure sysfs entries are synchronously removed before
> module memory is freed?

I would ignore this. The same code (just kobject_put()) is used in
samples/kobject/kobject-example.c which is supposed to show how
the API should be used.

It is just a test module. The interface is used only by the selftest.

I believe that this is a problem of the kobject API and should
be fixed there.

Best Regards,
Petr