* [PATCH v2 2/2] EDAC/pci_sysfs: Fix calling kobject_put() with ->state_initialized unset
2023-11-28 18:30 [PATCH v2 1/2] EDAC/device_sysfs: Fix calling kobject_put() with ->state_initialized unset Harshit Mogalapalli
@ 2023-11-28 18:30 ` Harshit Mogalapalli
2024-01-22 6:56 ` [PATCH v2 1/2] EDAC/device_sysfs: " Harshit Mogalapalli
2024-01-25 13:09 ` Borislav Petkov
2 siblings, 0 replies; 5+ messages in thread
From: Harshit Mogalapalli @ 2023-11-28 18:30 UTC (permalink / raw)
To: Borislav Petkov, Tony Luck, James Morse, Mauro Carvalho Chehab,
Robert Richter, Greg Kroah-Hartman, linux-edac, linux-kernel
Cc: dan.carpenter, kernel-janitors, error27, harshit.m.mogalapalli, stable
In edac_pci_main_kobj_setup() when dev_root is NULL,
kobject_init_and_add() is not called.
if (err) { // err = -ENODEV;
edac_dbg(1, "Failed to register '.../edac/pci'\n");
goto kobject_init_and_add_fail; // call to kobject_put()
}
This will cause a runtime warning in kobject_put() if the above happens.
Warning:
"kobject: '%s' (%p): is not initialized, yet kobject_put() is being called."
Fix the error handling to avoid the above possible situation.
Cc: <stable@vger.kernel.org>
Fixes: cb4a0bec0bb9 ("EDAC/sysfs: move to use bus_get_dev_root()")
Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
---
This is based on static analysis and only compile tested.
v1->v2: Resend as a patchset as they fix two similar bugs with modified
subject.
---
drivers/edac/edac_pci_sysfs.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/edac/edac_pci_sysfs.c b/drivers/edac/edac_pci_sysfs.c
index 287cc51dbc86..185ae9eda8a5 100644
--- a/drivers/edac/edac_pci_sysfs.c
+++ b/drivers/edac/edac_pci_sysfs.c
@@ -370,12 +370,14 @@ static int edac_pci_main_kobj_setup(void)
/* Instanstiate the pci object */
dev_root = bus_get_dev_root(edac_subsys);
- if (dev_root) {
- err = kobject_init_and_add(edac_pci_top_main_kobj,
- &ktype_edac_pci_main_kobj,
- &dev_root->kobj, "pci");
- put_device(dev_root);
- }
+ if (!dev_root)
+ goto kzalloc_fail;
+
+ err = kobject_init_and_add(edac_pci_top_main_kobj,
+ &ktype_edac_pci_main_kobj,
+ &dev_root->kobj, "pci");
+ put_device(dev_root);
+
if (err) {
edac_dbg(1, "Failed to register '.../edac/pci'\n");
goto kobject_init_and_add_fail;
--
2.42.0
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2 1/2] EDAC/device_sysfs: Fix calling kobject_put() with ->state_initialized unset
2023-11-28 18:30 [PATCH v2 1/2] EDAC/device_sysfs: Fix calling kobject_put() with ->state_initialized unset Harshit Mogalapalli
2023-11-28 18:30 ` [PATCH v2 2/2] EDAC/pci_sysfs: " Harshit Mogalapalli
@ 2024-01-22 6:56 ` Harshit Mogalapalli
2024-01-25 13:09 ` Borislav Petkov
2 siblings, 0 replies; 5+ messages in thread
From: Harshit Mogalapalli @ 2024-01-22 6:56 UTC (permalink / raw)
To: Borislav Petkov, Tony Luck, James Morse, Mauro Carvalho Chehab,
Robert Richter, Greg Kroah-Hartman, linux-edac, linux-kernel
Cc: dan.carpenter, kernel-janitors, error27, stable
Hi,
On 29/11/23 12:00 am, Harshit Mogalapalli wrote:
> In edac_device_register_sysfs_main_kobj(), when dev_root is NULL,
> kobject_init_and_add() is not called.
>
> if (err) { // err = -ENODEV
> edac_dbg(1, "Failed to register '.../edac/%s'\n",
> edac_dev->name);
> goto err_kobj_reg; // This calls kobj_put()
> }
>
> This will cause a runtime warning in kobject_put() if the above happens.
> Warning:
> "kobject: '%s' (%p): is not initialized, yet kobject_put() is being called."
>
> Fix the error handling to avoid the above possible situation.
>
> Cc: <stable@vger.kernel.org>
> Fixes: cb4a0bec0bb9 ("EDAC/sysfs: move to use bus_get_dev_root()")
> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
Ping on this two patch series:
Lore url:
https://lore.kernel.org/all/20231128183037.3395755-1-harshit.m.mogalapalli@oracle.com/
Thanks,
Harshit
> ---
> This is based on static analysis and only compile tested.
> v1->v2: Resend as a patchset as they are two similar bugs.
> ---
> drivers/edac/edac_device_sysfs.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/edac/edac_device_sysfs.c b/drivers/edac/edac_device_sysfs.c
> index 010c26be5846..4cac14cbdb60 100644
> --- a/drivers/edac/edac_device_sysfs.c
> +++ b/drivers/edac/edac_device_sysfs.c
> @@ -253,11 +253,13 @@ int edac_device_register_sysfs_main_kobj(struct edac_device_ctl_info *edac_dev)
>
> /* register */
> dev_root = bus_get_dev_root(edac_subsys);
> - if (dev_root) {
> - err = kobject_init_and_add(&edac_dev->kobj, &ktype_device_ctrl,
> - &dev_root->kobj, "%s", edac_dev->name);
> - put_device(dev_root);
> - }
> + if (!dev_root)
> + goto module_put;
> +
> + err = kobject_init_and_add(&edac_dev->kobj, &ktype_device_ctrl,
> + &dev_root->kobj, "%s", edac_dev->name);
> + put_device(dev_root);
> +
> if (err) {
> edac_dbg(1, "Failed to register '.../edac/%s'\n",
> edac_dev->name);
> @@ -276,8 +278,8 @@ int edac_device_register_sysfs_main_kobj(struct edac_device_ctl_info *edac_dev)
> /* Error exit stack */
> err_kobj_reg:
> kobject_put(&edac_dev->kobj);
> +module_put:
> module_put(edac_dev->owner);
> -
> err_out:
> return err;
> }
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2 1/2] EDAC/device_sysfs: Fix calling kobject_put() with ->state_initialized unset
2023-11-28 18:30 [PATCH v2 1/2] EDAC/device_sysfs: Fix calling kobject_put() with ->state_initialized unset Harshit Mogalapalli
2023-11-28 18:30 ` [PATCH v2 2/2] EDAC/pci_sysfs: " Harshit Mogalapalli
2024-01-22 6:56 ` [PATCH v2 1/2] EDAC/device_sysfs: " Harshit Mogalapalli
@ 2024-01-25 13:09 ` Borislav Petkov
2024-01-25 14:20 ` Harshit Mogalapalli
2 siblings, 1 reply; 5+ messages in thread
From: Borislav Petkov @ 2024-01-25 13:09 UTC (permalink / raw)
To: Harshit Mogalapalli
Cc: Tony Luck, James Morse, Mauro Carvalho Chehab, Robert Richter,
Greg Kroah-Hartman, linux-edac, linux-kernel, dan.carpenter,
kernel-janitors, error27, stable
On Tue, Nov 28, 2023 at 10:30:35AM -0800, Harshit Mogalapalli wrote:
> In edac_device_register_sysfs_main_kobj(), when dev_root is NULL,
When is dev_root NULL?
A real use case or this is just from code staring?
> diff --git a/drivers/edac/edac_device_sysfs.c b/drivers/edac/edac_device_sysfs.c
> index 010c26be5846..4cac14cbdb60 100644
> --- a/drivers/edac/edac_device_sysfs.c
> +++ b/drivers/edac/edac_device_sysfs.c
> @@ -253,11 +253,13 @@ int edac_device_register_sysfs_main_kobj(struct edac_device_ctl_info *edac_dev)
>
> /* register */
> dev_root = bus_get_dev_root(edac_subsys);
> - if (dev_root) {
> - err = kobject_init_and_add(&edac_dev->kobj, &ktype_device_ctrl,
> - &dev_root->kobj, "%s", edac_dev->name);
What cb4a0bec0bb9 did looks wrong. That if (err) check should be inside
the if (dev_root).
IOW, that function's error checking needs balancing. Something like
this below.
And looking at the other one, it has the same issue...
---
diff --git a/drivers/edac/edac_device_sysfs.c b/drivers/edac/edac_device_sysfs.c
index 237a542e045a..3476ef13e681 100644
--- a/drivers/edac/edac_device_sysfs.c
+++ b/drivers/edac/edac_device_sysfs.c
@@ -228,8 +228,8 @@ static struct kobj_type ktype_device_ctrl = {
*/
int edac_device_register_sysfs_main_kobj(struct edac_device_ctl_info *edac_dev)
{
- struct device *dev_root;
const struct bus_type *edac_subsys;
+ struct device *dev_root;
int err = -ENODEV;
edac_dbg(1, "\n");
@@ -243,26 +243,26 @@ int edac_device_register_sysfs_main_kobj(struct edac_device_ctl_info *edac_dev)
/* Init the devices's kobject */
memset(&edac_dev->kobj, 0, sizeof(struct kobject));
- /* Record which module 'owns' this control structure
- * and bump the ref count of the module
+ /*
+ * Record which module 'owns' this control structure and bump
+ * the ref count of the module
*/
edac_dev->owner = THIS_MODULE;
if (!try_module_get(edac_dev->owner))
goto err_out;
- /* register */
dev_root = bus_get_dev_root(edac_subsys);
- if (dev_root) {
- err = kobject_init_and_add(&edac_dev->kobj, &ktype_device_ctrl,
- &dev_root->kobj, "%s", edac_dev->name);
- put_device(dev_root);
- }
+ if (!dev_root)
+ goto err_module;
+
+ err = kobject_init_and_add(&edac_dev->kobj, &ktype_device_ctrl,
+ &dev_root->kobj, "%s", edac_dev->name);
if (err) {
- edac_dbg(1, "Failed to register '.../edac/%s'\n",
- edac_dev->name);
+ edac_dbg(1, "Failed to register '.../edac/%s'\n", edac_dev->name);
goto err_kobj_reg;
}
+
kobject_uevent(&edac_dev->kobj, KOBJ_ADD);
/* At this point, to 'free' the control struct,
@@ -273,9 +273,11 @@ int edac_device_register_sysfs_main_kobj(struct edac_device_ctl_info *edac_dev)
return 0;
- /* Error exit stack */
err_kobj_reg:
kobject_put(&edac_dev->kobj);
+ put_device(dev_root);
+
+err_module:
module_put(edac_dev->owner);
err_out:
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2 1/2] EDAC/device_sysfs: Fix calling kobject_put() with ->state_initialized unset
2024-01-25 13:09 ` Borislav Petkov
@ 2024-01-25 14:20 ` Harshit Mogalapalli
0 siblings, 0 replies; 5+ messages in thread
From: Harshit Mogalapalli @ 2024-01-25 14:20 UTC (permalink / raw)
To: Borislav Petkov
Cc: Tony Luck, James Morse, Mauro Carvalho Chehab, Robert Richter,
Greg Kroah-Hartman, linux-edac, linux-kernel, dan.carpenter,
kernel-janitors, error27, stable
On 25/01/24 6:39 pm, Borislav Petkov wrote:
> On Tue, Nov 28, 2023 at 10:30:35AM -0800, Harshit Mogalapalli wrote:
>> In edac_device_register_sysfs_main_kobj(), when dev_root is NULL,
>
Thanks for checking this.
> When is dev_root NULL?
>
> A real use case or this is just from code staring?
>
This is based on static analysis, not real testing.
>> diff --git a/drivers/edac/edac_device_sysfs.c b/drivers/edac/edac_device_sysfs.c
>> index 010c26be5846..4cac14cbdb60 100644
>> --- a/drivers/edac/edac_device_sysfs.c
>> +++ b/drivers/edac/edac_device_sysfs.c
>> @@ -253,11 +253,13 @@ int edac_device_register_sysfs_main_kobj(struct edac_device_ctl_info *edac_dev)
>>
>> /* register */
>> dev_root = bus_get_dev_root(edac_subsys);
>> - if (dev_root) {
>> - err = kobject_init_and_add(&edac_dev->kobj, &ktype_device_ctrl,
>> - &dev_root->kobj, "%s", edac_dev->name);
>
> What cb4a0bec0bb9 did looks wrong. That if (err) check should be inside
> the if (dev_root).
>
> IOW, that function's error checking needs balancing. Something like
> this below.
>
> And looking at the other one, it has the same issue...
>
> ---
> diff --git a/drivers/edac/edac_device_sysfs.c b/drivers/edac/edac_device_sysfs.c
> index 237a542e045a..3476ef13e681 100644
> --- a/drivers/edac/edac_device_sysfs.c
> +++ b/drivers/edac/edac_device_sysfs.c
> @@ -228,8 +228,8 @@ static struct kobj_type ktype_device_ctrl = {
> */
> int edac_device_register_sysfs_main_kobj(struct edac_device_ctl_info *edac_dev)
> {
> - struct device *dev_root;
> const struct bus_type *edac_subsys;
> + struct device *dev_root;
> int err = -ENODEV;
>
> edac_dbg(1, "\n");
> @@ -243,26 +243,26 @@ int edac_device_register_sysfs_main_kobj(struct edac_device_ctl_info *edac_dev)
> /* Init the devices's kobject */
> memset(&edac_dev->kobj, 0, sizeof(struct kobject));
>
> - /* Record which module 'owns' this control structure
> - * and bump the ref count of the module
> + /*
> + * Record which module 'owns' this control structure and bump
> + * the ref count of the module
> */
> edac_dev->owner = THIS_MODULE;
>
> if (!try_module_get(edac_dev->owner))
> goto err_out;
>
> - /* register */
> dev_root = bus_get_dev_root(edac_subsys);
> - if (dev_root) {
> - err = kobject_init_and_add(&edac_dev->kobj, &ktype_device_ctrl,
> - &dev_root->kobj, "%s", edac_dev->name);
> - put_device(dev_root);
> - }
> + if (!dev_root)
> + goto err_module;
> +
> + err = kobject_init_and_add(&edac_dev->kobj, &ktype_device_ctrl,
> + &dev_root->kobj, "%s", edac_dev->name);
> if (err) {
> - edac_dbg(1, "Failed to register '.../edac/%s'\n",
> - edac_dev->name);
> + edac_dbg(1, "Failed to register '.../edac/%s'\n", edac_dev->name);
> goto err_kobj_reg;
> }
> +
> kobject_uevent(&edac_dev->kobj, KOBJ_ADD);
>
> /* At this point, to 'free' the control struct,
> @@ -273,9 +273,11 @@ int edac_device_register_sysfs_main_kobj(struct edac_device_ctl_info *edac_dev)
>
> return 0;
>
> - /* Error exit stack */
> err_kobj_reg:
> kobject_put(&edac_dev->kobj);
> + put_device(dev_root);
In this diff, put_device(dev_root) is not called on the success path. I
checked couple of other callers of bus_get_dev_root() and they call
put_device(dev_root) after using dev_root. I think we need to have
put_device() on the success path as well.
> +
> +err_module:
> module_put(edac_dev->owner);
>
An alternate diff could be:
diff --git a/drivers/edac/edac_device_sysfs.c
b/drivers/edac/edac_device_sysfs.c
index 237a542e045a..0c8e66e40af8 100644
--- a/drivers/edac/edac_device_sysfs.c
+++ b/drivers/edac/edac_device_sysfs.c
@@ -275,7 +275,8 @@ int edac_device_register_sysfs_main_kobj(struct
edac_device_ctl_info *edac_dev)
/* Error exit stack */
err_kobj_reg:
- kobject_put(&edac_dev->kobj);
+ if(dev_root)
+ kobject_put(&edac_dev->kobj);
module_put(edac_dev->owner);
err_out:
Thanks,
Harshit
> err_out:
>
^ permalink raw reply [flat|nested] 5+ messages in thread