i3c: fix refcount inconsistency in i3c_master_register
PATCH 1/1 i3c: fix refcount inconsistency in i3c_master_register
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.
--- drivers/i3c/master.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/i3c/master.c +4 -1
@@ -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);
Signature · 7 lines
2.39.5 (Apple Git-154) -- linux-i3c mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-i3c
Frank Li
reviewerQuoted 21 lines · Shuhao Fu
In
i3c_master_register, a possible refcount inconsistency has been identified, causing possible resource leak.Function
of_node_getincreases the refcount ofparent->of_node. If functioni3c_bus_initfails, 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_initfails.Fixes 3a379bbcea0a ("i3c: Add core I3C infrastructure")Signed-off-by Shuhao Fu <[email protected]>--- drivers/i3c/master.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/i3c/master.c +0
-0
drivers/i3c/master.c int i3c_master_register(struct i3c_master_controller *master, · line 2888
Quoted 5 lines
drivers/i3c/master.c +1
-1@@ -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
Quoted 21 lines
drivers/i3c/master.c +3
-1device_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
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c
Quoted 31 lines · Frank Li
Quoted 21 lines · Shuhao Fu
In
i3c_master_register, a possible refcount inconsistency has been identified, causing possible resource leak.Function
of_node_getincreases the refcount ofparent->of_node. If functioni3c_bus_initfails, 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_initfails.Fixes 3a379bbcea0a ("i3c: Add core I3C infrastructure")Signed-off-by Shuhao Fu <[email protected]>--- drivers/i3c/master.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/i3c/master.c +0
-0drivers/i3c/master.c int i3c_master_register(struct i3c_master_controller *master, · line 2888
Quoted 5 lines
drivers/i3c/master.c +1
-1@@ -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_nodebecauseof_node_put()also missed ati3c_master_unregister()
Quoted 3 lines
you can refer
drivers/base/platform.cFrank
Do you mean that we should do of_node_release in platform_device_release, instead of respecting the refcounting via of_node_put?
Quoted 22 lines
Quoted 22 lines
drivers/i3c/master.c +3
-1device_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
[email protected]
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
Signature · 3 lines
linux-i3c mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-i3c
Frank Li
reviewerQuoted 32 lines · Shuhao Fu
Quoted 31 lines · Frank Li
Quoted 21 lines · Shuhao Fu
In
i3c_master_register, a possible refcount inconsistency has been identified, causing possible resource leak.Function
of_node_getincreases the refcount ofparent->of_node. If functioni3c_bus_initfails, 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_initfails.Fixes 3a379bbcea0a ("i3c: Add core I3C infrastructure")Signed-off-by Shuhao Fu <[email protected]>--- drivers/i3c/master.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/i3c/master.c +0
-0drivers/i3c/master.c int i3c_master_register(struct i3c_master_controller *master, · line 2888
Quoted 5 lines
drivers/i3c/master.c +1
-1@@ -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_nodebecauseof_node_put()also missed ati3c_master_unregister()
Quoted 7 lines
Quoted 3 lines
you can refer
drivers/base/platform.cFrank
Do you mean that we should do
of_node_releaseinplatform_device_release, instead of respecting the refcounting viaof_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
Quoted 28 lines
Quoted 19 lines
Quoted 19 lines
drivers/i3c/master.c +3
-1device_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)
Quoted 4 lines
Quoted 4 lines
--
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c--
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c
Quoted 56 lines · Frank Li
Quoted 32 lines · Shuhao Fu
Quoted 31 lines · Frank Li
Quoted 21 lines · Shuhao Fu
In
i3c_master_register, a possible refcount inconsistency has been identified, causing possible resource leak.Function
of_node_getincreases the refcount ofparent->of_node. If functioni3c_bus_initfails, 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_initfails.Fixes 3a379bbcea0a ("i3c: Add core I3C infrastructure")Signed-off-by Shuhao Fu <[email protected]>--- drivers/i3c/master.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/i3c/master.c +0
-0drivers/i3c/master.c int i3c_master_register(struct i3c_master_controller *master, · line 2888
Quoted 5 lines
drivers/i3c/master.c +1
-1@@ -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_nodebecauseof_node_put()also missed ati3c_master_unregister()Quoted 7 lines
Quoted 3 lines
you can refer
drivers/base/platform.cFrank
Do you mean that we should do
of_node_releaseinplatform_device_release, instead of respecting the refcounting viaof_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; ... };
Quoted 5 lines
Suppose
of_node_put()will be auto called whenput_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
Quoted 28 lines
Quoted 28 lines
Quoted 19 lines
Quoted 19 lines
drivers/i3c/master.c +3
-1device_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)
Quoted 4 lines
Quoted 4 lines
--
linux-i3c mailing list
[email protected]
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
[email protected]
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
Signature · 3 lines
linux-i3c mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-i3c
Frank Li
reviewerQuoted 61 lines · Shuhao Fu
Quoted 60 lines · Frank Li
Quoted 32 lines · Shuhao Fu
Quoted 31 lines · Frank Li
Quoted 21 lines · Shuhao Fu
In
i3c_master_register, a possible refcount inconsistency has been identified, causing possible resource leak.Function
of_node_getincreases the refcount ofparent->of_node. If functioni3c_bus_initfails, 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_initfails.Fixes 3a379bbcea0a ("i3c: Add core I3C infrastructure")Signed-off-by Shuhao Fu <[email protected]>--- drivers/i3c/master.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/i3c/master.c +0
-0drivers/i3c/master.c int i3c_master_register(struct i3c_master_controller *master, · line 2888
Quoted 5 lines
drivers/i3c/master.c +1
-1@@ -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_nodebecauseof_node_put()also missed ati3c_master_unregister()Quoted 7 lines
Quoted 3 lines
you can refer
drivers/base/platform.cFrank
Do you mean that we should do
of_node_releaseinplatform_device_release, instead of respecting the refcounting viaof_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 whenput_device(&master->dev);Do you really meet the problem or just static anaysis?
Quoted 8 lines
Quoted 1 line
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_deviceif it fails. I also checked call sites related toi3c_master_registerand 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
Quoted 30 lines
Shuhao
Quoted 28 lines
Quoted 28 lines
Quoted 19 lines
Quoted 19 lines
drivers/i3c/master.c +3
-1device_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)
Quoted 4 lines
Quoted 4 lines
--
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c--
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c
Quoted 95 lines · Frank Li
Quoted 69 lines · Shuhao Fu
Quoted 56 lines · Frank Li
Quoted 32 lines · Shuhao Fu
Quoted 31 lines · Frank Li
Quoted 21 lines · Shuhao Fu
In
i3c_master_register, a possible refcount inconsistency has been identified, causing possible resource leak.Function
of_node_getincreases the refcount ofparent->of_node. If functioni3c_bus_initfails, 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_initfails.Fixes 3a379bbcea0a ("i3c: Add core I3C infrastructure")Signed-off-by Shuhao Fu <[email protected]>--- drivers/i3c/master.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/i3c/master.c +0
-0drivers/i3c/master.c int i3c_master_register(struct i3c_master_controller *master, · line 2888
Quoted 5 lines
drivers/i3c/master.c +1
-1@@ -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_nodebecauseof_node_put()also missed ati3c_master_unregister()Quoted 7 lines
Quoted 3 lines
you can refer
drivers/base/platform.cFrank
Do you mean that we should do
of_node_releaseinplatform_device_release, instead of respecting the refcounting viaof_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; ... };Quoted 5 lines
Suppose
of_node_put()will be auto called whenput_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_deviceif it fails. I also checked call sites related toi3c_master_registerand they dont seem to do the clean-up if register fails.(quoted diff) +4
-4@@ -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().
Quoted 2 lines
Frank
Thank you for fixing the refcounting issue. May I kindly ask for a reported-by tag for this fix "Reported-by: Shuhao Fu <[email protected]>"?
Thanks,
Shuhao
Quoted 30 lines
Quoted 30 lines
Shuhao
Quoted 28 lines
Quoted 28 lines
Quoted 19 lines
Quoted 19 lines
drivers/i3c/master.c +3
-1device_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)
Quoted 4 lines
Quoted 4 lines
--
linux-i3c mailing list
[email protected]
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
[email protected]
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
Signature · 3 lines
linux-i3c mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-i3c