linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] i3c: fix refcount inconsistency in i3c_master_register
@ 2025-10-08  7:27 Shuhao Fu
  2025-10-09 16:17 ` Frank Li
  0 siblings, 1 reply; 7+ messages in thread
From: Shuhao Fu @ 2025-10-08  7:27 UTC (permalink / raw)
  To: Alexandre Belloni; +Cc: Frank Li, linux-i3c, linux-kernel

In `i3c_master_register`, a possible refcount inconsistency has been
identified, causing possible resource leak.

Function `of_node_get` increases the refcount of `parent->of_node`. If 
function `i3c_bus_init` fails, the function returns immediately without
a corresponding decrease, resulting in an inconsistent refcounter.

In this patch, an extra goto label is added to ensure the balance of
refcount when `i3c_bus_init` fails.

Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
Signed-off-by: Shuhao Fu <sfual@cse.ust.hk>
---
 drivers/i3c/master.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index d946db75d..9f4fe98d2 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -2885,7 +2885,7 @@ int i3c_master_register(struct i3c_master_controller *master,
 
 	ret = i3c_bus_init(i3cbus, master->dev.of_node);
 	if (ret)
-		return ret;
+		goto err_put_of_node;
 
 	device_initialize(&master->dev);
 	dev_set_name(&master->dev, "i3c-%d", i3cbus->id);
@@ -2973,6 +2973,9 @@ int i3c_master_register(struct i3c_master_controller *master,
 err_put_dev:
 	put_device(&master->dev);
 
+err_put_of_node:
+	of_node_put(master->dev.of_node);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(i3c_master_register);
-- 
2.39.5 (Apple Git-154)


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] i3c: fix refcount inconsistency in i3c_master_register
  2025-10-08  7:27 [PATCH] i3c: fix refcount inconsistency in i3c_master_register Shuhao Fu
@ 2025-10-09 16:17 ` Frank Li
  2025-10-10  6:34   ` Shuhao Fu
  0 siblings, 1 reply; 7+ messages in thread
From: Frank Li @ 2025-10-09 16:17 UTC (permalink / raw)
  To: Shuhao Fu; +Cc: Alexandre Belloni, linux-i3c, linux-kernel

On Wed, Oct 08, 2025 at 03:27:09PM +0800, Shuhao Fu wrote:
> In `i3c_master_register`, a possible refcount inconsistency has been
> identified, causing possible resource leak.
>
> Function `of_node_get` increases the refcount of `parent->of_node`. If
> function `i3c_bus_init` fails, the function returns immediately without
> a corresponding decrease, resulting in an inconsistent refcounter.
>
> In this patch, an extra goto label is added to ensure the balance of
> refcount when `i3c_bus_init` fails.
>
> Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
> Signed-off-by: Shuhao Fu <sfual@cse.ust.hk>
> ---
>  drivers/i3c/master.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index d946db75d..9f4fe98d2 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -2885,7 +2885,7 @@ int i3c_master_register(struct i3c_master_controller *master,
>
>  	ret = i3c_bus_init(i3cbus, master->dev.of_node);
>  	if (ret)
> -		return ret;
> +		goto err_put_of_node;

I think it'd better to set release function for master dev to release
of_node because of_node_put() also missed at i3c_master_unregister()

you can refer drivers/base/platform.c

Frank

>
>  	device_initialize(&master->dev);
>  	dev_set_name(&master->dev, "i3c-%d", i3cbus->id);
> @@ -2973,6 +2973,9 @@ int i3c_master_register(struct i3c_master_controller *master,
>  err_put_dev:
>  	put_device(&master->dev);
>
> +err_put_of_node:
> +	of_node_put(master->dev.of_node);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(i3c_master_register);
> --
> 2.39.5 (Apple Git-154)
>
>
> --
> linux-i3c mailing list
> linux-i3c@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-i3c

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] i3c: fix refcount inconsistency in i3c_master_register
  2025-10-09 16:17 ` Frank Li
@ 2025-10-10  6:34   ` Shuhao Fu
  2025-10-13 20:19     ` Frank Li
  0 siblings, 1 reply; 7+ messages in thread
From: Shuhao Fu @ 2025-10-10  6:34 UTC (permalink / raw)
  To: Frank Li; +Cc: Alexandre Belloni, linux-i3c, linux-kernel

On Thu, Oct 09, 2025 at 12:17:11PM -0400, Frank Li wrote:
> On Wed, Oct 08, 2025 at 03:27:09PM +0800, Shuhao Fu wrote:
> > In `i3c_master_register`, a possible refcount inconsistency has been
> > identified, causing possible resource leak.
> >
> > Function `of_node_get` increases the refcount of `parent->of_node`. If
> > function `i3c_bus_init` fails, the function returns immediately without
> > a corresponding decrease, resulting in an inconsistent refcounter.
> >
> > In this patch, an extra goto label is added to ensure the balance of
> > refcount when `i3c_bus_init` fails.
> >
> > Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
> > Signed-off-by: Shuhao Fu <sfual@cse.ust.hk>
> > ---
> >  drivers/i3c/master.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index d946db75d..9f4fe98d2 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
> > @@ -2885,7 +2885,7 @@ int i3c_master_register(struct i3c_master_controller *master,
> >
> >  	ret = i3c_bus_init(i3cbus, master->dev.of_node);
> >  	if (ret)
> > -		return ret;
> > +		goto err_put_of_node;
> 
> I think it'd better to set release function for master dev to release
> of_node because of_node_put() also missed at i3c_master_unregister()
> 
> you can refer drivers/base/platform.c
> 
> Frank

Do you mean that we should do `of_node_release` in
`platform_device_release`, instead of respecting the refcounting via
`of_node_put`? 

> 
> >
> >  	device_initialize(&master->dev);
> >  	dev_set_name(&master->dev, "i3c-%d", i3cbus->id);
> > @@ -2973,6 +2973,9 @@ int i3c_master_register(struct i3c_master_controller *master,
> >  err_put_dev:
> >  	put_device(&master->dev);
> >
> > +err_put_of_node:
> > +	of_node_put(master->dev.of_node);
> > +
> >  	return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(i3c_master_register);
> > --
> > 2.39.5 (Apple Git-154)
> >
> >
> > --
> > linux-i3c mailing list
> > linux-i3c@lists.infradead.org
> > https://apc01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infradead.org%2Fmailman%2Flistinfo%2Flinux-i3c&data=05%7C02%7Csfual%40connect.ust.hk%7Cbfee3f910b654276330508de074f5750%7C6c1d415239d044ca88d9b8d6ddca0708%7C1%7C0%7C638956234525799989%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=aTeKcQJDC2Av%2FQ1MjtDuoOYrWJ6ZwhbjQbHs%2Fci90jw%3D&reserved=0

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] i3c: fix refcount inconsistency in i3c_master_register
  2025-10-10  6:34   ` Shuhao Fu
@ 2025-10-13 20:19     ` Frank Li
  2025-10-13 21:09       ` Shuhao Fu
  0 siblings, 1 reply; 7+ messages in thread
From: Frank Li @ 2025-10-13 20:19 UTC (permalink / raw)
  To: Shuhao Fu; +Cc: Alexandre Belloni, linux-i3c, linux-kernel

On Fri, Oct 10, 2025 at 02:34:08PM +0800, Shuhao Fu wrote:
> On Thu, Oct 09, 2025 at 12:17:11PM -0400, Frank Li wrote:
> > On Wed, Oct 08, 2025 at 03:27:09PM +0800, Shuhao Fu wrote:
> > > In `i3c_master_register`, a possible refcount inconsistency has been
> > > identified, causing possible resource leak.
> > >
> > > Function `of_node_get` increases the refcount of `parent->of_node`. If
> > > function `i3c_bus_init` fails, the function returns immediately without
> > > a corresponding decrease, resulting in an inconsistent refcounter.
> > >
> > > In this patch, an extra goto label is added to ensure the balance of
> > > refcount when `i3c_bus_init` fails.
> > >
> > > Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
> > > Signed-off-by: Shuhao Fu <sfual@cse.ust.hk>
> > > ---
> > >  drivers/i3c/master.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > > index d946db75d..9f4fe98d2 100644
> > > --- a/drivers/i3c/master.c
> > > +++ b/drivers/i3c/master.c
> > > @@ -2885,7 +2885,7 @@ int i3c_master_register(struct i3c_master_controller *master,
> > >
> > >  	ret = i3c_bus_init(i3cbus, master->dev.of_node);
> > >  	if (ret)
> > > -		return ret;
> > > +		goto err_put_of_node;
> >
> > I think it'd better to set release function for master dev to release
> > of_node because of_node_put() also missed at i3c_master_unregister()
> >
> > you can refer drivers/base/platform.c
> >
> > Frank
>
> Do you mean that we should do `of_node_release` in
> `platform_device_release`, instead of respecting the refcounting via
> `of_node_put`?

Sorry, I checked code again.

static void i3c_masterdev_release(struct device *dev)
{
        ...
        of_node_put(dev->of_node);
}

i3c_master_register()
{
	...
	master->dev.release = i3c_masterdev_release;
	...
};

Suppose of_node_put() will be auto called when put_device(&master->dev);

Do you really meet the problem or just static anaysis?

Frank
>
> >
> > >
> > >  	device_initialize(&master->dev);
> > >  	dev_set_name(&master->dev, "i3c-%d", i3cbus->id);
> > > @@ -2973,6 +2973,9 @@ int i3c_master_register(struct i3c_master_controller *master,
> > >  err_put_dev:
> > >  	put_device(&master->dev);
> > >
> > > +err_put_of_node:
> > > +	of_node_put(master->dev.of_node);
> > > +
> > >  	return ret;
> > >  }
> > >  EXPORT_SYMBOL_GPL(i3c_master_register);
> > > --
> > > 2.39.5 (Apple Git-154)
> > >
> > >
> > > --
> > > linux-i3c mailing list
> > > linux-i3c@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-i3c
>
> --
> linux-i3c mailing list
> linux-i3c@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-i3c

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] i3c: fix refcount inconsistency in i3c_master_register
  2025-10-13 20:19     ` Frank Li
@ 2025-10-13 21:09       ` Shuhao Fu
  2025-10-13 22:01         ` Frank Li
  0 siblings, 1 reply; 7+ messages in thread
From: Shuhao Fu @ 2025-10-13 21:09 UTC (permalink / raw)
  To: Frank Li; +Cc: Alexandre Belloni, linux-i3c, linux-kernel

On Mon, Oct 13, 2025 at 04:19:00PM -0400, Frank Li wrote:
> On Fri, Oct 10, 2025 at 02:34:08PM +0800, Shuhao Fu wrote:
> > On Thu, Oct 09, 2025 at 12:17:11PM -0400, Frank Li wrote:
> > > On Wed, Oct 08, 2025 at 03:27:09PM +0800, Shuhao Fu wrote:
> > > > In `i3c_master_register`, a possible refcount inconsistency has been
> > > > identified, causing possible resource leak.
> > > >
> > > > Function `of_node_get` increases the refcount of `parent->of_node`. If
> > > > function `i3c_bus_init` fails, the function returns immediately without
> > > > a corresponding decrease, resulting in an inconsistent refcounter.
> > > >
> > > > In this patch, an extra goto label is added to ensure the balance of
> > > > refcount when `i3c_bus_init` fails.
> > > >
> > > > Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
> > > > Signed-off-by: Shuhao Fu <sfual@cse.ust.hk>
> > > > ---
> > > >  drivers/i3c/master.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > > > index d946db75d..9f4fe98d2 100644
> > > > --- a/drivers/i3c/master.c
> > > > +++ b/drivers/i3c/master.c
> > > > @@ -2885,7 +2885,7 @@ int i3c_master_register(struct i3c_master_controller *master,
> > > >
> > > >  	ret = i3c_bus_init(i3cbus, master->dev.of_node);
> > > >  	if (ret)
> > > > -		return ret;
> > > > +		goto err_put_of_node;
> > >
> > > I think it'd better to set release function for master dev to release
> > > of_node because of_node_put() also missed at i3c_master_unregister()
> > >
> > > you can refer drivers/base/platform.c
> > >
> > > Frank
> >
> > Do you mean that we should do `of_node_release` in
> > `platform_device_release`, instead of respecting the refcounting via
> > `of_node_put`?
> 
> Sorry, I checked code again.
> 
> static void i3c_masterdev_release(struct device *dev)
> {
>         ...
>         of_node_put(dev->of_node);
> }
> 
> i3c_master_register()
> {
> 	...
> 	master->dev.release = i3c_masterdev_release;
> 	...
> };
> 
> Suppose of_node_put() will be auto called when put_device(&master->dev);
> 
> Do you really meet the problem or just static anaysis?
> 
> Frank

Honestly, it's from static analysis.

My apologies for overlooking the release handle. I checked the code once
again. It still looks suspicious as it would not call `put_device` if it
fails. I also checked call sites related to `i3c_master_register` and
they dont seem to do the clean-up if register fails.

Shuhao
> >
> > >
> > > >
> > > >  	device_initialize(&master->dev);
> > > >  	dev_set_name(&master->dev, "i3c-%d", i3cbus->id);
> > > > @@ -2973,6 +2973,9 @@ int i3c_master_register(struct i3c_master_controller *master,
> > > >  err_put_dev:
> > > >  	put_device(&master->dev);
> > > >
> > > > +err_put_of_node:
> > > > +	of_node_put(master->dev.of_node);
> > > > +
> > > >  	return ret;
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(i3c_master_register);
> > > > --
> > > > 2.39.5 (Apple Git-154)
> > > >
> > > >
> > > > --
> > > > linux-i3c mailing list
> > > > linux-i3c@lists.infradead.org
> > > > https://apc01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infradead.org%2Fmailman%2Flistinfo%2Flinux-i3c&data=05%7C02%7Csfual%40connect.ust.hk%7C837a825f1f3443950e6a08de0a95cb5f%7C6c1d415239d044ca88d9b8d6ddca0708%7C1%7C0%7C638959835659671475%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=B0ZvY%2FDKW3VdG%2FVamjGUUg%2BVr%2BZbtHIf4otgBKhje1s%3D&reserved=0
> >
> > --
> > linux-i3c mailing list
> > linux-i3c@lists.infradead.org
> > https://apc01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infradead.org%2Fmailman%2Flistinfo%2Flinux-i3c&data=05%7C02%7Csfual%40connect.ust.hk%7C837a825f1f3443950e6a08de0a95cb5f%7C6c1d415239d044ca88d9b8d6ddca0708%7C1%7C0%7C638959835659696068%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=KpMM%2FsTa6G1o3q%2Bcqx6iTb3VbCDq723lCXgcA9GGetI%3D&reserved=0

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] i3c: fix refcount inconsistency in i3c_master_register
  2025-10-13 21:09       ` Shuhao Fu
@ 2025-10-13 22:01         ` Frank Li
  2025-10-14  1:55           ` Shuhao Fu
  0 siblings, 1 reply; 7+ messages in thread
From: Frank Li @ 2025-10-13 22:01 UTC (permalink / raw)
  To: Shuhao Fu; +Cc: Alexandre Belloni, linux-i3c, linux-kernel

On Tue, Oct 14, 2025 at 05:09:53AM +0800, Shuhao Fu wrote:
> On Mon, Oct 13, 2025 at 04:19:00PM -0400, Frank Li wrote:
> > On Fri, Oct 10, 2025 at 02:34:08PM +0800, Shuhao Fu wrote:
> > > On Thu, Oct 09, 2025 at 12:17:11PM -0400, Frank Li wrote:
> > > > On Wed, Oct 08, 2025 at 03:27:09PM +0800, Shuhao Fu wrote:
> > > > > In `i3c_master_register`, a possible refcount inconsistency has been
> > > > > identified, causing possible resource leak.
> > > > >
> > > > > Function `of_node_get` increases the refcount of `parent->of_node`. If
> > > > > function `i3c_bus_init` fails, the function returns immediately without
> > > > > a corresponding decrease, resulting in an inconsistent refcounter.
> > > > >
> > > > > In this patch, an extra goto label is added to ensure the balance of
> > > > > refcount when `i3c_bus_init` fails.
> > > > >
> > > > > Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
> > > > > Signed-off-by: Shuhao Fu <sfual@cse.ust.hk>
> > > > > ---
> > > > >  drivers/i3c/master.c | 5 ++++-
> > > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > > > > index d946db75d..9f4fe98d2 100644
> > > > > --- a/drivers/i3c/master.c
> > > > > +++ b/drivers/i3c/master.c
> > > > > @@ -2885,7 +2885,7 @@ int i3c_master_register(struct i3c_master_controller *master,
> > > > >
> > > > >  	ret = i3c_bus_init(i3cbus, master->dev.of_node);
> > > > >  	if (ret)
> > > > > -		return ret;
> > > > > +		goto err_put_of_node;
> > > >
> > > > I think it'd better to set release function for master dev to release
> > > > of_node because of_node_put() also missed at i3c_master_unregister()
> > > >
> > > > you can refer drivers/base/platform.c
> > > >
> > > > Frank
> > >
> > > Do you mean that we should do `of_node_release` in
> > > `platform_device_release`, instead of respecting the refcounting via
> > > `of_node_put`?
> >
> > Sorry, I checked code again.
> >
> > static void i3c_masterdev_release(struct device *dev)
> > {
> >         ...
> >         of_node_put(dev->of_node);
> > }
> >
> > i3c_master_register()
> > {
> > 	...
> > 	master->dev.release = i3c_masterdev_release;
> > 	...
> > };
> >
> > Suppose of_node_put() will be auto called when put_device(&master->dev);
> >
> > Do you really meet the problem or just static anaysis?
> >
> > Frank
>
> Honestly, it's from static analysis.
>
> My apologies for overlooking the release handle. I checked the code once
> again. It still looks suspicious as it would not call `put_device` if it
> fails. I also checked call sites related to `i3c_master_register` and
> they dont seem to do the clean-up if register fails.


@@ -2814,10 +2816,6 @@ int i3c_master_register(struct i3c_master_controller *master,
        INIT_LIST_HEAD(&master->boardinfo.i2c);
        INIT_LIST_HEAD(&master->boardinfo.i3c);

-       ret = i3c_bus_init(i3cbus, master->dev.of_node);
-       if (ret)
-               return ret;
-
        device_initialize(&master->dev);
        dev_set_name(&master->dev, "i3c-%d", i3cbus->id);

@@ -2825,6 +2823,10 @@ int i3c_master_register(struct i3c_master_controller *master,
        master->dev.coherent_dma_mask = parent->coherent_dma_mask;
        master->dev.dma_parms = parent->dma_parms;

+       ret = i3c_bus_init(i3cbus, master->dev.of_node);
+        if (ret)
+                goto err_put_dev;
+

I inject at error at i3c_bus_init(), above code can trigger i3c_masterdev_release,
which call of_node_put().

Frank

>
> Shuhao
> > >
> > > >
> > > > >
> > > > >  	device_initialize(&master->dev);
> > > > >  	dev_set_name(&master->dev, "i3c-%d", i3cbus->id);
> > > > > @@ -2973,6 +2973,9 @@ int i3c_master_register(struct i3c_master_controller *master,
> > > > >  err_put_dev:
> > > > >  	put_device(&master->dev);
> > > > >
> > > > > +err_put_of_node:
> > > > > +	of_node_put(master->dev.of_node);
> > > > > +
> > > > >  	return ret;
> > > > >  }
> > > > >  EXPORT_SYMBOL_GPL(i3c_master_register);
> > > > > --
> > > > > 2.39.5 (Apple Git-154)
> > > > >
> > > > >
> > > > > --
> > > > > linux-i3c mailing list
> > > > > linux-i3c@lists.infradead.org
> > > > > http://lists.infradead.org/mailman/listinfo/linux-i3c
> > >
> > > --
> > > linux-i3c mailing list
> > > linux-i3c@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-i3c

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] i3c: fix refcount inconsistency in i3c_master_register
  2025-10-13 22:01         ` Frank Li
@ 2025-10-14  1:55           ` Shuhao Fu
  0 siblings, 0 replies; 7+ messages in thread
From: Shuhao Fu @ 2025-10-14  1:55 UTC (permalink / raw)
  To: Frank Li; +Cc: Alexandre Belloni, linux-i3c, linux-kernel

On Mon, Oct 13, 2025 at 06:01:19PM -0400, Frank Li wrote:
> On Tue, Oct 14, 2025 at 05:09:53AM +0800, Shuhao Fu wrote:
> > On Mon, Oct 13, 2025 at 04:19:00PM -0400, Frank Li wrote:
> > > On Fri, Oct 10, 2025 at 02:34:08PM +0800, Shuhao Fu wrote:
> > > > On Thu, Oct 09, 2025 at 12:17:11PM -0400, Frank Li wrote:
> > > > > On Wed, Oct 08, 2025 at 03:27:09PM +0800, Shuhao Fu wrote:
> > > > > > In `i3c_master_register`, a possible refcount inconsistency has been
> > > > > > identified, causing possible resource leak.
> > > > > >
> > > > > > Function `of_node_get` increases the refcount of `parent->of_node`. If
> > > > > > function `i3c_bus_init` fails, the function returns immediately without
> > > > > > a corresponding decrease, resulting in an inconsistent refcounter.
> > > > > >
> > > > > > In this patch, an extra goto label is added to ensure the balance of
> > > > > > refcount when `i3c_bus_init` fails.
> > > > > >
> > > > > > Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
> > > > > > Signed-off-by: Shuhao Fu <sfual@cse.ust.hk>
> > > > > > ---
> > > > > >  drivers/i3c/master.c | 5 ++++-
> > > > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > > > > > index d946db75d..9f4fe98d2 100644
> > > > > > --- a/drivers/i3c/master.c
> > > > > > +++ b/drivers/i3c/master.c
> > > > > > @@ -2885,7 +2885,7 @@ int i3c_master_register(struct i3c_master_controller *master,
> > > > > >
> > > > > >  	ret = i3c_bus_init(i3cbus, master->dev.of_node);
> > > > > >  	if (ret)
> > > > > > -		return ret;
> > > > > > +		goto err_put_of_node;
> > > > >
> > > > > I think it'd better to set release function for master dev to release
> > > > > of_node because of_node_put() also missed at i3c_master_unregister()
> > > > >
> > > > > you can refer drivers/base/platform.c
> > > > >
> > > > > Frank
> > > >
> > > > Do you mean that we should do `of_node_release` in
> > > > `platform_device_release`, instead of respecting the refcounting via
> > > > `of_node_put`?
> > >
> > > Sorry, I checked code again.
> > >
> > > static void i3c_masterdev_release(struct device *dev)
> > > {
> > >         ...
> > >         of_node_put(dev->of_node);
> > > }
> > >
> > > i3c_master_register()
> > > {
> > > 	...
> > > 	master->dev.release = i3c_masterdev_release;
> > > 	...
> > > };
> > >
> > > Suppose of_node_put() will be auto called when put_device(&master->dev);
> > >
> > > Do you really meet the problem or just static anaysis?
> > >
> > > Frank
> >
> > Honestly, it's from static analysis.
> >
> > My apologies for overlooking the release handle. I checked the code once
> > again. It still looks suspicious as it would not call `put_device` if it
> > fails. I also checked call sites related to `i3c_master_register` and
> > they dont seem to do the clean-up if register fails.
> 
> 
> @@ -2814,10 +2816,6 @@ int i3c_master_register(struct i3c_master_controller *master,
>         INIT_LIST_HEAD(&master->boardinfo.i2c);
>         INIT_LIST_HEAD(&master->boardinfo.i3c);
> 
> -       ret = i3c_bus_init(i3cbus, master->dev.of_node);
> -       if (ret)
> -               return ret;
> -
>         device_initialize(&master->dev);
>         dev_set_name(&master->dev, "i3c-%d", i3cbus->id);
> 
> @@ -2825,6 +2823,10 @@ int i3c_master_register(struct i3c_master_controller *master,
>         master->dev.coherent_dma_mask = parent->coherent_dma_mask;
>         master->dev.dma_parms = parent->dma_parms;
> 
> +       ret = i3c_bus_init(i3cbus, master->dev.of_node);
> +        if (ret)
> +                goto err_put_dev;
> +
> 
> I inject at error at i3c_bus_init(), above code can trigger i3c_masterdev_release,
> which call of_node_put().
> 
> Frank
> 

Thank you for fixing the refcounting issue. May I kindly ask for a
reported-by tag for this fix "Reported-by: Shuhao Fu <sfual@cse.ust.hk>"?

Thanks,
Shuhao

> >
> > Shuhao
> > > >
> > > > >
> > > > > >
> > > > > >  	device_initialize(&master->dev);
> > > > > >  	dev_set_name(&master->dev, "i3c-%d", i3cbus->id);
> > > > > > @@ -2973,6 +2973,9 @@ int i3c_master_register(struct i3c_master_controller *master,
> > > > > >  err_put_dev:
> > > > > >  	put_device(&master->dev);
> > > > > >
> > > > > > +err_put_of_node:
> > > > > > +	of_node_put(master->dev.of_node);
> > > > > > +
> > > > > >  	return ret;
> > > > > >  }
> > > > > >  EXPORT_SYMBOL_GPL(i3c_master_register);
> > > > > > --
> > > > > > 2.39.5 (Apple Git-154)
> > > > > >
> > > > > >
> > > > > > --
> > > > > > linux-i3c mailing list
> > > > > > linux-i3c@lists.infradead.org
> > > > > > https://apc01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infradead.org%2Fmailman%2Flistinfo%2Flinux-i3c&data=05%7C02%7Csfual%40connect.ust.hk%7Cdbe4f1ecc0c84f304fca08de0aa414ff%7C6c1d415239d044ca88d9b8d6ddca0708%7C1%7C0%7C638959897018898845%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=%2BmCDBh4d6Se%2BuO5xJgSDVupMRir7ZFH7f8RtzGUucoE%3D&reserved=0
> > > >
> > > > --
> > > > linux-i3c mailing list
> > > > linux-i3c@lists.infradead.org
> > > > https://apc01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infradead.org%2Fmailman%2Flistinfo%2Flinux-i3c&data=05%7C02%7Csfual%40connect.ust.hk%7Cdbe4f1ecc0c84f304fca08de0aa414ff%7C6c1d415239d044ca88d9b8d6ddca0708%7C1%7C0%7C638959897018922222%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=MdqJwKEKImd8UAQ0hyHjWyZx8vX1YSxU%2FqKDgpF0JPA%3D&reserved=0

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2025-10-14  1:55 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-08  7:27 [PATCH] i3c: fix refcount inconsistency in i3c_master_register Shuhao Fu
2025-10-09 16:17 ` Frank Li
2025-10-10  6:34   ` Shuhao Fu
2025-10-13 20:19     ` Frank Li
2025-10-13 21:09       ` Shuhao Fu
2025-10-13 22:01         ` Frank Li
2025-10-14  1:55           ` Shuhao Fu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).