[PATCH] kobject: fix double kobject_put in kobject_unregister()

[PATCH] kobject: fix double kobject_put in kobject_unregister()

[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 <anil.s.keshavamurthy@intel.com>


 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
--- linux-2.6.10-rc1-mm4/lib/kobject.c~kobject_unregister_fix	2004-11-10 13:43:42.243877455 -0800
+++ linux-2.6.10-rc1-mm4-askeshav/lib/kobject.c	2004-11-10 13:46:30.788797265 -0800
@@ -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);
 }
 
 /**
_

Re: [PATCH] kobject: fix double kobject_put in kobject_unregister()

[Russell King]

On Wed, Nov 10, 2004 at 02:19:23PM -0800, Keshavamurthy Anil S wrote:
> 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.

-- 
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

Re: [PATCH] kobject: fix double kobject_put in kobject_unregister()

[Keshavamurthy Anil S]

On Wed, Nov 10, 2004 at 10:30:16PM +0000, Russell King wrote:
> On Wed, Nov 10, 2004 at 02:19:23PM -0800, Keshavamurthy Anil S wrote:
> > 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.



> 
> IOW, the sequence:
> 
> 	allocate
> 	register	(refcount >= 2 after this completes)
> 	unregister
> 
> will automatically free the object once the last user has gone.
> 
> -- 
> 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

Re: [PATCH] kobject: fix double kobject_put in kobject_unregister()

[Greg KH]

On Wed, Nov 10, 2004 at 02:19:23PM -0800, Keshavamurthy Anil S wrote:
> 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 <anil.s.keshavamurthy@intel.com>
> 
> 
>  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
> --- linux-2.6.10-rc1-mm4/lib/kobject.c~kobject_unregister_fix	2004-11-10 13:43:42.243877455 -0800
> +++ linux-2.6.10-rc1-mm4-askeshav/lib/kobject.c	2004-11-10 13:46:30.788797265 -0800
> @@ -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);
>  }

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

Re: [PATCH] kobject: fix double kobject_put in kobject_unregister()

[Keshavamurthy Anil S]

On Wed, Nov 10, 2004 at 02:54:21PM -0800, Greg KH wrote:
> On Wed, Nov 10, 2004 at 02:19:23PM -0800, Keshavamurthy Anil S wrote:
> > 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 <anil.s.keshavamurthy@intel.com>
> > 
> > 
> >  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
> > --- linux-2.6.10-rc1-mm4/lib/kobject.c~kobject_unregister_fix	2004-11-10 13:43:42.243877455 -0800
> > +++ linux-2.6.10-rc1-mm4-askeshav/lib/kobject.c	2004-11-10 13:46:30.788797265 -0800
> > @@ -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);
> >  }
> 
> 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?

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

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



> 
> thanks,
> 
> gre k-h

Re: [PATCH] kobject: fix double kobject_put in kobject_unregister()

[Keiichiro Tokunaga]

On Wed, 10 Nov 2004 15:04:01 -0800 Keshavamurthy Anil S wrote:
> On Wed, Nov 10, 2004 at 02:54:21PM -0800, Greg KH wrote:
> > On Wed, Nov 10, 2004 at 02:19:23PM -0800, Keshavamurthy Anil S wrote:
> > > 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 <anil.s.keshavamurthy@intel.com>
> > > 
> > > 
> > >  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
> > > --- linux-2.6.10-rc1-mm4/lib/kobject.c~kobject_unregister_fix	2004-11-10 13:43:42.243877455 -0800
> > > +++ linux-2.6.10-rc1-mm4-askeshav/lib/kobject.c	2004-11-10 13:46:30.788797265 -0800
> > > @@ -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);
> > >  }
> > 
> > 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?
> 
> 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
> 
> 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 <tokunaga.keiich@jp.fujitsu.com>
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(-)

diff -puN drivers/acpi/scan.c~fix_acpi_device_register drivers/acpi/scan.c
--- linux-2.6.10-rc1-mm4-kei/drivers/acpi/scan.c~fix_acpi_device_register	2004-11-11 10:11:26.121101371 +0900
+++ linux-2.6.10-rc1-mm4-kei-kei/drivers/acpi/scan.c	2004-11-11 10:11:39.126072074 +0900
@@ -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);
 }
 

_

Re: [PATCH] kobject: fix double kobject_put in kobject_unregister()

[Keshavamurthy Anil S]

On Wed, Nov 10, 2004 at 02:54:21PM -0800, Greg KH wrote:
> 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.


> 
> 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 <anil.s.keshavamurthy@intel.com>
---

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

diff -puN drivers/acpi/scan.c~kobject_register_fix1 drivers/acpi/scan.c
--- linux-2.6.10-rc1-mm4/drivers/acpi/scan.c~kobject_register_fix1	2004-11-10 17:40:23.393117553 -0800
+++ linux-2.6.10-rc1-mm4-askeshav/drivers/acpi/scan.c	2004-11-10 17:41:45.844288418 -0800
@@ -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);
 }
 
_