siox: fix master memory leak on registration failure

[PATCH] siox: fix master memory leak on registration failure

Posted by Guangshuo Li 5 days, 3 hours ago

siox_master_register() takes an additional reference on the master
device with get_device() so that the SIOX core owns a reference until
siox_master_unregister() is called.

If kthread_run() fails, siox_master_register() returns without dropping
this reference. Similarly, if device_add() fails, the poll thread is
stopped but the reference acquired by siox_master_register() is not
released.

For devm-allocated masters, the devres cleanup only drops the original
allocation reference. The additional registration reference therefore
remains held, preventing siox_master_release() from being called and
leaking the siox_master allocation.

Drop the reference acquired by siox_master_register() on both failure
paths. On device_add() failure, kthread_stop() first lets the poll thread
drop its own reference before the registration reference is released.

The issue was identified by a static analysis tool I developed and
confirmed by manual review.

Fixes: 2c12932b8e65 ("siox: Don't pass the reference on a master in siox_master_register()")
Cc: stable@vger.kernel.org
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 drivers/siox/siox-core.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/siox/siox-core.c b/drivers/siox/siox-core.c
index 3e8f3b6a4555..ea1ef0a5c991 100644
--- a/drivers/siox/siox-core.c
+++ b/drivers/siox/siox-core.c
@@ -753,14 +753,21 @@ int siox_master_register(struct siox_master *smaster)
 	smaster->poll_thread = kthread_run(siox_poll_thread, smaster,
 					   "siox-%d", smaster->busno);
 	if (IS_ERR(smaster->poll_thread)) {
+		ret = PTR_ERR(smaster->poll_thread);
 		smaster->active = 0;
-		return PTR_ERR(smaster->poll_thread);
+		goto err_put_device;
 	}
 
 	ret = device_add(&smaster->dev);
-	if (ret)
+	if (ret) {
 		kthread_stop(smaster->poll_thread);
+		goto err_put_device;
+	}
 
+	return 0;
+
+err_put_device:
+	put_device(&smaster->dev);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(siox_master_register);
-- 
2.43.0

Re: [PATCH] siox: fix master memory leak on registration failure

Posted by Markus Elfring 3 days, 10 hours ago

…
> +++ b/drivers/siox/siox-core.c
> @@ -753,14 +753,21 @@ int siox_master_register(struct siox_master *smaster)
>  	smaster->poll_thread = kthread_run(siox_poll_thread, smaster,
>  					   "siox-%d", smaster->busno);
>  	if (IS_ERR(smaster->poll_thread)) {
> +		ret = PTR_ERR(smaster->poll_thread);

Would it be nicer to use this variable assignment directly before
the goto statement?


>  		smaster->active = 0;
> -		return PTR_ERR(smaster->poll_thread);
> +		goto err_put_device;
>  	}
…


Regards,
Markus

Re: [PATCH] siox: fix master memory leak on registration failure

Posted by Uwe Kleine-König 3 days, 8 hours ago

On Mon, Sep 21, 2026 at 12:51:00PM +0200, Markus Elfring wrote:
> …
> > +++ b/drivers/siox/siox-core.c
> > @@ -753,14 +753,21 @@ int siox_master_register(struct siox_master *smaster)
> >  	smaster->poll_thread = kthread_run(siox_poll_thread, smaster,
> >  					   "siox-%d", smaster->busno);
> >  	if (IS_ERR(smaster->poll_thread)) {
> > +		ret = PTR_ERR(smaster->poll_thread);
> 
> Would it be nicer to use this variable assignment directly before
> the goto statement?

No, please don't, IMHO it's fine to have IS_ERR and PTR_ERR together.

But I wonder if it's sensible to create a function to all the usual init
stuff such that the error handling in siox_master_register() can become
just:

	get_device(&smaster->dev);

	ret = siox_master_init(...);
	if (ret)
		put_device();

	return ret;

Having said that, I wonder about the smaster->active = 0 assignment.
It's quite some time ago that I wrote that code, but either it's
useless (that's where my bet is on), or this assignment is missing in
the error path of device_add()? Thorsten?

Best regards
Uwe

Re: [PATCH] siox: fix master memory leak on registration failure

Posted by Thorsten Scherer 8 hours ago

Hello everyone,

thank you for the patch and the input.

On Mon, Sep 21, 2026 at 03:02:01PM +0200, Uwe Kleine-König wrote:
> On Mon, Sep 21, 2026 at 12:51:00PM +0200, Markus Elfring wrote:
> > …
> > > +++ b/drivers/siox/siox-core.c
> > > @@ -753,14 +753,21 @@ int siox_master_register(struct siox_master *smaster)
> > >  	smaster->poll_thread = kthread_run(siox_poll_thread, smaster,
> > >  					   "siox-%d", smaster->busno);
> > >  	if (IS_ERR(smaster->poll_thread)) {
> > > +		ret = PTR_ERR(smaster->poll_thread);
> > 
> > Would it be nicer to use this variable assignment directly before
> > the goto statement?
> 
> No, please don't, IMHO it's fine to have IS_ERR and PTR_ERR together.
> 
> But I wonder if it's sensible to create a function to all the usual init
> stuff such that the error handling in siox_master_register() can become
> just:
> 
> 	get_device(&smaster->dev);
> 
> 	ret = siox_master_init(...);
> 	if (ret)
> 		put_device();
> 
> 	return ret;
> 
> Having said that, I wonder about the smaster->active = 0 assignment.
> It's quite some time ago that I wrote that code, but either it's
> useless (that's where my bet is on), or this assignment is missing in
> the error path of device_add()? Thorsten?

At a first glance I'm with you.  Assignment doesn't change anything.
But I don't have a clear view (on the whole context) yet.  The patch
seems legit and fixes a real issue.  Having said that,  it seems that
there is an additional imbalance at siox_poll_thread.

I will have to take a little time to swap siox in and get a clearer picture.

Coming back to you soon.

> Best regards
> Uwe

Best regards
Thorsten