kobject: fix double kobject_put in kobject_unregister()

PATCH In review Keshavamurthy Anil S 4 participants 7 messages Nov 10, 2004 – Nov 11, 2004

Subsystems (3)

ACPI Library code Driver core

lore

Share & tools

lore

Keshavamurthy Anil S

author

2004-11-10 22:19 UTC #0 lore raw

PATCH 1/1 kobject: fix double kobject_put in kobject_unregister()

Hi Greg,

This patch fixes the problem where in kobject resources were getting freed when those kobject were still in use due to double kobject_put() getting called in the kobject_unregister() code path.

With out this patch kobject_unregister() will have some serious side effects.

Please apply.

Signed-off-by Anil S Keshavamurthy <[email protected]>

 linux-2.6.10-rc1-mm4-askeshav/lib/kobject.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)

lib/kobject.c renamed from lib/kobject.c~kobject_unregister_fix +2 -1

diff -puN lib/kobject.c~kobject_unregister_fix lib/kobject.c
@@ -301,6 +301,8 @@ void kobject_del(struct kobject * kobj)
 {
 	kobject_hotplug(kobj, KOBJ_REMOVE);
 	sysfs_remove_dir(kobj);
+
+	/* unlink does kobject_put() for us */
 	unlink(kobj);
 }

@@ -313,7 +315,6 @@ void kobject_unregister(struct kobject *
 {
 	pr_debug("kobject %s: unregistering\n",kobject_name(kobj));
 	kobject_del(kobj);
-	kobject_put(kobj);
 }
 
 /**

-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_idU88&alloc_id065&op=click
_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Russell King

2004-11-10 22:30 UTC #1 lore raw

Quoted 5 lines · Keshavamurthy Anil S

Hi Greg,
This patch fixes the problem where in kobject resources were getting freed when those kobject were still in use due to double kobject_put() getting called in the kobject_unregister() code path.

Isn't it intended that, after an sysfs/kobject/device object is unregistered that the thread doing the unregistering must not dereference the memory associated with that object?

IOW, the sequence:

allocate
register (refcount >= 2 after this completes)
unregister

will automatically free the object once the last user has gone.

Signature · 15 lines

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core


-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_idU88&alloc_id065&op=click
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Keshavamurthy Anil S

author

2004-11-10 22:39 UTC #2 lore raw

Quoted 10 lines · Russell King

Quoted 5 lines · Keshavamurthy Anil S
Hi Greg,
This patch fixes the problem where in kobject resources were getting freed when those kobject were still in use due to double kobject_put() getting called in the kobject_unregister() code path.
Isn't it intended that, after an sysfs/kobject/device object is unregistered that the thread doing the unregistering must not dereference the memory associated with that object?

Yes, nobody is touching the resource once it is freed. But because of
double kobject_put() getting called in the kobject_unregister() code patch
I am seeing successive parent's kobject_unregister() causing panic as that kobject's kobject_cleanup() gets called when childs kobject_unregiser() is getting called. i.e patern's kobject_cleanup() gets called for some reason when clild's kobject_unregister() is called. My patch fixes this issues.

Quoted 14 lines

IOW, the sequence:
allocate
register (refcount >= 2 after this completes)
unregister
will automatically free the object once the last user has gone.
Signature · 4 lines
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Greg KH

2004-11-10 22:54 UTC #3 lore raw

In reply to Keshavamurthy Anil S

Quoted 29 lines · Keshavamurthy Anil S

Hi Greg,
This patch fixes the problem where in kobject resources were getting freed when those kobject were still in use due to double kobject_put() getting called in the kobject_unregister() code path.
With out this patch kobject_unregister() will have some serious side effects.
Please apply.
Signed-off-by Anil S Keshavamurthy <[email protected]>
 linux-2.6.10-rc1-mm4-askeshav/lib/kobject.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)
diff -puN lib/kobject.c~kobject_unregister_fix lib/kobject.c
lib/kobject.c renamed from lib/kobject.c~kobject_unregister_fix +2 -0
@@ -301,6 +301,8 @@ void kobject_del(struct kobject * kobj)
 {
 	kobject_hotplug(kobj, KOBJ_REMOVE);
 	sysfs_remove_dir(kobj);
+
+	/* unlink does kobject_put() for us */
 	unlink(kobj);
 }
 

lib/kobject.c void kobject_unregister(struct kobject * · line 318

Quoted 5 lines

lib/kobject.c renamed from lib/kobject.c~kobject_unregister_fix +0 -1

@@ -313,7 +315,6 @@ void kobject_unregister(struct kobject *
 {
 	pr_debug("kobject %s: unregistering\n",kobject_name(kobj));
 	kobject_del(kobj);
-	kobject_put(kobj);
 }

No, this is wrong. Count the add and put in the sequence of:
kobject_register()
kobject_unregister()

they are balanced.

You mention you are seeing problems. Have a trace? Example code?

thanks,

gre k-h

Keshavamurthy Anil S

author

2004-11-10 23:04 UTC #4 lore raw

Quoted 40 lines · Greg KH

Quoted 29 lines · Keshavamurthy Anil S
Hi Greg,
This patch fixes the problem where in kobject resources were getting freed when those kobject were still in use due to double kobject_put() getting called in the kobject_unregister() code path.
With out this patch kobject_unregister() will have some serious side effects.
Please apply.
Signed-off-by Anil S Keshavamurthy <[email protected]>
 linux-2.6.10-rc1-mm4-askeshav/lib/kobject.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)
diff -puN lib/kobject.c~kobject_unregister_fix lib/kobject.c
lib/kobject.c renamed from lib/kobject.c~kobject_unregister_fix +2 -0
@@ -301,6 +301,8 @@ void kobject_del(struct kobject * kobj)
 {
 	kobject_hotplug(kobj, KOBJ_REMOVE);
 	sysfs_remove_dir(kobj);
+
+	/* unlink does kobject_put() for us */
 	unlink(kobj);
 }
 
lib/kobject.c void kobject_unregister(struct kobject * · line 318
Quoted 5 lines
lib/kobject.c renamed from lib/kobject.c~kobject_unregister_fix +0 -1
@@ -313,7 +315,6 @@ void kobject_unregister(struct kobject *
 {
 	pr_debug("kobject %s: unregistering\n",kobject_name(kobj));
 	kobject_del(kobj);
-	kobject_put(kobj);
 }
No, this is wrong. Count the add and put in the sequence of:
kobject_register()
kobject_unregister()

Quoted 3 lines

they are balanced.
You mention you are seeing problems. Have a trace? Example code?

Here is the call trace when I tried to do kobject_unregiser()

Call Trace:
 [<a000000100016de0>] show_stack+0x80/0xa0
                                spà0000000e3cf730 bspà0000000e3c9230
 [<a000000100017640>] show_regs+0x840/0x880
                                spà0000000e3cf900 bspà0000000e3c91c8
 [<a000000100023f10>] die+0x150/0x1c0
                                spà0000000e3cf910 bspà0000000e3c9188
 [<a000000100023fc0>] die_if_kernel+0x40/0x60
                                spà0000000e3cf910 bspà0000000e3c9158
 [<a0000001000240f0>] ia64_fault+0x110/0x1060
                                spà0000000e3cf910 bspà0000000e3c9100
 [<a00000010000dae0>] ia64_leave_kernel+0x0/0x260
                                spà0000000e3cfb20 bspà0000000e3c9100
 [<a0000001002743e0>] strlen+0x20/0x140
                                spà0000000e3cfcf0 bspà0000000e3c90a8
 [<a00000010026a270>] kobject_get_path+0x150/0x1e0
                                spà0000000e3cfcf0 bspà0000000e3c9068
 [<a00000010026a770>] kobject_hotplug+0x350/0x6e0
                                spà0000000e3cfcf0 bspà0000000e3c9008
 [<a0000001002695b0>] kobject_del+0x30/0x80
                                spà0000000e3cfd10 bspà0000000e3c8fe0
 [<a000000100269620>] kobject_unregister+0x20/0x60
                                spà0000000e3cfd10 bspà0000000e3c8fc0
 [<a000000100302420>] acpi_device_unregister+0x320/0x340
                                spà0000000e3cfd10 bspà0000000e3c8fa0
 [<a000000100302b70>] acpi_bus_remove+0x2f0/0x340
                                spà0000000e3cfd10 bspà0000000e3c8f68
 [<a000000100302ec0>] acpi_bus_trim+0x300/0x440
                                spà0000000e3cfd30 bspà0000000e3c8f18
 [<a000000100303110>] acpi_eject_store+0x110/0x240
                                spà0000000e3cfde0 bspà0000000e3c8ee0
 [<a000000100301fb0>] acpi_device_attr_store+0x70/0xa0
                                spà0000000e3cfe20 bspà0000000e3c8ea8
 [<a0000001001864b0>] sysfs_write_file+0x270/0x300
                                spà0000000e3cfe20 bspà0000000e3c8e58
 [<a0000001001052e0>] vfs_write+0x200/0x2c0

I will try to explain the what is happening here.

I have a sysfs tree like this.
.../_SB/LSB0
.../_SB/LSB0/CPU5
.../_SB/LSB0/MEM1
.../_SB/LSB0/MEM2

Say LSB0 is kobj1, CPU5 is kobj2, MEM1 is kobj3, MEM2 is kobj4

All I am trying to do is first I will do kobject_unregister(kobj2),then kobject_unregister(kobj3), then kobject_unregister(kobj4) and now when I try to do kobject_unregister(kobj1) i.e when I am trying to remove LSB0 directory, I am seeing the above stack trace.

The patch I posted fixed this problem.

Let me know if you need more data.

-Anil

Quoted 4 lines

thanks,
gre k-h

Keiichiro Tokunaga

2004-11-11 02:03 UTC #5 lore raw

Quoted 93 lines · On Wed, 10 Nov 2004 15:04:01 -0800 Keshavamurthy Anil S wrote:

Quoted 40 lines · Greg KH

Quoted 29 lines · Keshavamurthy Anil S
Hi Greg,
This patch fixes the problem where in kobject resources were getting freed when those kobject were still in use due to double kobject_put() getting called in the kobject_unregister() code path.
With out this patch kobject_unregister() will have some serious side effects.
Please apply.
Signed-off-by Anil S Keshavamurthy <[email protected]>
 linux-2.6.10-rc1-mm4-askeshav/lib/kobject.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)
diff -puN lib/kobject.c~kobject_unregister_fix lib/kobject.c
lib/kobject.c renamed from lib/kobject.c~kobject_unregister_fix +2 -0
@@ -301,6 +301,8 @@ void kobject_del(struct kobject * kobj)
 {
 	kobject_hotplug(kobj, KOBJ_REMOVE);
 	sysfs_remove_dir(kobj);
+
+	/* unlink does kobject_put() for us */
 	unlink(kobj);
 }
 
lib/kobject.c void kobject_unregister(struct kobject * · line 318
Quoted 5 lines
lib/kobject.c renamed from lib/kobject.c~kobject_unregister_fix +0 -1
@@ -313,7 +315,6 @@ void kobject_unregister(struct kobject *
 {
 	pr_debug("kobject %s: unregistering\n",kobject_name(kobj));
 	kobject_del(kobj);
-	kobject_put(kobj);
 }
No, this is wrong. Count the add and put in the sequence of:
kobject_register()
kobject_unregister()

Quoted 3 lines

they are balanced.
You mention you are seeing problems. Have a trace? Example code?

Here is the call trace when I tried to do kobject_unregiser()

Call Trace:
 [<a000000100016de0>] show_stack+0x80/0xa0
                                sp=e00000000e3cf730 bsp=e00000000e3c9230
 [<a000000100017640>] show_regs+0x840/0x880
                                sp=e00000000e3cf900 bsp=e00000000e3c91c8
 [<a000000100023f10>] die+0x150/0x1c0
                                sp=e00000000e3cf910 bsp=e00000000e3c9188
 [<a000000100023fc0>] die_if_kernel+0x40/0x60
                                sp=e00000000e3cf910 bsp=e00000000e3c9158
 [<a0000001000240f0>] ia64_fault+0x110/0x1060
                                sp=e00000000e3cf910 bsp=e00000000e3c9100
 [<a00000010000dae0>] ia64_leave_kernel+0x0/0x260
                                sp=e00000000e3cfb20 bsp=e00000000e3c9100
 [<a0000001002743e0>] strlen+0x20/0x140
                                sp=e00000000e3cfcf0 bsp=e00000000e3c90a8
 [<a00000010026a270>] kobject_get_path+0x150/0x1e0
                                sp=e00000000e3cfcf0 bsp=e00000000e3c9068
 [<a00000010026a770>] kobject_hotplug+0x350/0x6e0
                                sp=e00000000e3cfcf0 bsp=e00000000e3c9008
 [<a0000001002695b0>] kobject_del+0x30/0x80
                                sp=e00000000e3cfd10 bsp=e00000000e3c8fe0
 [<a000000100269620>] kobject_unregister+0x20/0x60
                                sp=e00000000e3cfd10 bsp=e00000000e3c8fc0
 [<a000000100302420>] acpi_device_unregister+0x320/0x340
                                sp=e00000000e3cfd10 bsp=e00000000e3c8fa0
 [<a000000100302b70>] acpi_bus_remove+0x2f0/0x340
                                sp=e00000000e3cfd10 bsp=e00000000e3c8f68
 [<a000000100302ec0>] acpi_bus_trim+0x300/0x440
                                sp=e00000000e3cfd30 bsp=e00000000e3c8f18
 [<a000000100303110>] acpi_eject_store+0x110/0x240
                                sp=e00000000e3cfde0 bsp=e00000000e3c8ee0
 [<a000000100301fb0>] acpi_device_attr_store+0x70/0xa0
                                sp=e00000000e3cfe20 bsp=e00000000e3c8ea8
 [<a0000001001864b0>] sysfs_write_file+0x270/0x300
                                sp=e00000000e3cfe20 bsp=e00000000e3c8e58
 [<a0000001001052e0>] vfs_write+0x200/0x2c0

I will try to explain the what is happening here.

I have a sysfs tree like this.
.../_SB/LSB0
.../_SB/LSB0/CPU5
.../_SB/LSB0/MEM1
.../_SB/LSB0/MEM2

Say LSB0 is kobj1, CPU5 is kobj2, MEM1 is kobj3, MEM2 is kobj4

Quoted 8 lines

All I am trying to do is first I will do kobject_unregister(kobj2),then kobject_unregister(kobj3), then kobject_unregister(kobj4) and now when I try to do kobject_unregister(kobj1) i.e when I am trying to remove LSB0 directory, I am seeing the above stack trace.
The patch I posted fixed this problem.
Let me know if you need more data.

I think the cause is an imbalance between acpi_device_register()
and acpi_device_unregister(). acpi_device_register() is supposed to increment a refcount of a target kobject (kobj1) and also a kobject (kobj2) that inherited its kset to the kobj1, but it doesn't today. This is because the kset is not set to kobj1 before calling kboject_init() for kobj1. On the other hand, acpi_device_unregister() decrements the refcount of both kobj1 and kobj2. Thus, kobj2 would be released unexpectedly if kobjects using the kset of kobj2 are released.

        kobject_init(&device->kobj);
        strlcpy(device->kobj.name,device->pnp.bus_id,KOBJ_NAME_LEN);
        if (parent)
                device->kobj.parent = &parent->kobj;
        device->kobj.ktype = &ktype_acpi_ns;
        device->kobj.kset = &acpi_namespace_kset;  <-- kset is set here.
        kobject_add(&device->kobj);

I am attaching a patch fixing this. I have added acpi-devel in cc
since this might be an ACPI issue. Excuse my cross posting...

Thanks,
Keiichiro Tokunaga

Signed-off-by Keiichiro Tokunaga <[email protected]>

Status: Tested on 2.6.10-rc1-mm4
---

 linux-2.6.10-rc1-mm4-kei-kei/drivers/acpi/scan.c |    3 +--
 1 files changed, 1 insertion(+), 2 deletions(-)

drivers/acpi/scan.c renamed from drivers/acpi/scan.c~fix_acpi_device_register +1 -2

diff -puN drivers/acpi/scan.c~fix_acpi_device_register drivers/acpi/scan.c
@@ -112,13 +112,12 @@ static void acpi_device_register(struct 
 		list_add_tail(&device->wakeup_list,&acpi_wakeup_device_list);
 	spin_unlock(&acpi_device_lock);
 
-	kobject_init(&device->kobj);
 	strlcpy(device->kobj.name,device->pnp.bus_id,KOBJ_NAME_LEN);
 	if (parent)
 		device->kobj.parent = &parent->kobj;
 	device->kobj.ktype = &ktype_acpi_ns;
 	device->kobj.kset = &acpi_namespace_kset;
-	kobject_add(&device->kobj);
+	kobject_register(&device->kobj);
 	create_sysfs_device_files(device);
 }

Keshavamurthy Anil S

author

2004-11-11 01:54 UTC #6 lore raw

In reply to Greg KH

Quoted 6 lines · Greg KH

On Wed, Nov 10, 2004 at 02:19:23PM -0800, Keshavamurthy Anil S wrote:
No, this is wrong. Count the add and put in the sequence of:
kobject_register()
kobject_unregister()
they are balanced.

Yes, I agree now, but after investigating further here is what I have found. If you think this is good, please ack this patch.

Quoted 2 lines

You mention you are seeing problems. Have a trace? Example code?

Here is what exactly happening. Please see the patch.
In ACPI, kobject_init() is called without initializing kobj.kset. Due to this kset.kobj's refcount does not get incremented. Again when we try to do kobject_unregister(), kset of the kobject that is getting unregister is obtained and kset's kobject refcount is decremented which was at teh first place never got inceremened. Due to this kset's kobj's refcount is decremented with out getting incremented and due to this bug, this kset.kobj is getting released.

Below fix fixes this problem. If you find this this Good patch, please ack this patch. I am copying to Len too on this patch.

Signed-of-by Anil S Keshavamurthy <[email protected]>

---

 linux-2.6.10-rc1-mm4-askeshav/drivers/acpi/scan.c |    3 +--
 1 files changed, 1 insertion(+), 2 deletions(-)

drivers/acpi/scan.c renamed from drivers/acpi/scan.c~kobject_register_fix1 +1 -2

diff -puN drivers/acpi/scan.c~kobject_register_fix1 drivers/acpi/scan.c
@@ -112,13 +112,12 @@ static void acpi_device_register(struct 
 		list_add_tail(&device->wakeup_list,&acpi_wakeup_device_list);
 	spin_unlock(&acpi_device_lock);
 
-	kobject_init(&device->kobj);
 	strlcpy(device->kobj.name,device->pnp.bus_id,KOBJ_NAME_LEN);
 	if (parent)
 		device->kobj.parent = &parent->kobj;
 	device->kobj.ktype = &ktype_acpi_ns;
 	device->kobj.kset = &acpi_namespace_kset;
-	kobject_add(&device->kobj);
+	kobject_register(&device->kobj);
 	create_sysfs_device_files(device);
 }