[PATCH} fix defect with kobject memory leaks during del_gendisk

[PATCH} fix defect with kobject memory leaks during del_gendisk

[Steven Dake]

[-- Attachment #1: Type: text/plain, Size: 936 bytes --]

Folks,

Attached is a patch which fixes a few memory leaks in 2.6.0-test5. 
Comments are welcome as to whether or not this patch is the right
solution.

I added the ability to linux MD to generate add and remove hotplug calls
on RAID START and STOPs.  To achieve this, I use the del_gendisk call
which deletes the gendisk, delete the children kobjects, and delete the
parent (in this case, mdX) kobject.

Unfortunately it appears that del_gendisk uses kobject_del to delete the
kobject.  If the kobject has a ktype release function, it is not called
in the kobject_del call path, but only in kobject_unregister.

This patch changes the functionality so the release function (in this
case the block device release function in drivers/block/genhd.c) is
called by changing the kobject_del to kobject_unregister.

Without this patch, at least 3 memory leaks occur on removal of a block
device using the del_gendisk function.

Thanks
-steve

[-- Attachment #2: fix_del_gendisk_kobj.patch --]
[-- Type: text/plain, Size: 337 bytes --]

--- linux-2.6.0-test5/fs/partitions/check.c	2003-09-08 12:50:28.000000000 -0700
+++ linux-fixmd/fs/partitions/check.c	2003-09-24 15:28:57.000000000 -0700
@@ -458,5 +458,5 @@
 		sysfs_remove_link(&disk->driverfs_dev->kobj, "block");
 		put_device(disk->driverfs_dev);
 	}
-	kobject_del(&disk->kobj);
+	kobject_unregister(&disk->kobj);
 }

Re: [PATCH} fix defect with kobject memory leaks during del_gendisk

[Christoph Hellwig]

On Wed, Sep 24, 2003 at 04:02:06PM -0700, Steven Dake wrote:
> Unfortunately it appears that del_gendisk uses kobject_del to delete the
> kobject.  If the kobject has a ktype release function, it is not called
> in the kobject_del call path, but only in kobject_unregister.

That's intentional.  gendisks (like everything using kobjects) are
reference counted and ->release is unly called after the last reference
goes away, for gendisks that would be the last put_disk call.

Unless you miss the put_disk call (which md certainly has) there's
no memeory leak.

Re: [PATCH} fix defect with kobject memory leaks during del_gendisk

[Patrick Mochel]

> Attached is a patch which fixes a few memory leaks in 2.6.0-test5. 
> Comments are welcome as to whether or not this patch is the right
> solution.
> 
> I added the ability to linux MD to generate add and remove hotplug calls
> on RAID START and STOPs.  To achieve this, I use the del_gendisk call
> which deletes the gendisk, delete the children kobjects, and delete the
> parent (in this case, mdX) kobject.
> 
> Unfortunately it appears that del_gendisk uses kobject_del to delete the
> kobject.  If the kobject has a ktype release function, it is not called
> in the kobject_del call path, but only in kobject_unregister.
> 
> This patch changes the functionality so the release function (in this
> case the block device release function in drivers/block/genhd.c) is
> called by changing the kobject_del to kobject_unregister.

It is not the right thing to do, as Christoph pointed out. Gendisks have a 
lifetime longer than the time between add_disk() and del_gendisk(). They 
are created before they are added, and the objects may persist longer 
after they have been removed from view. 

To delete the last reference, the MD layer should be calling put_disk(), 
which will trigger release to happen. 

Your patch will actually cause other bugs, since the final put_disk() of 
other block drivers will now be called on already freed memory.


	Pat

Re: [PATCH} fix defect with kobject memory leaks during del_gendisk

[Steven Dake]

On Wed, 2003-09-24 at 23:23, Christoph Hellwig wrote:
> On Wed, Sep 24, 2003 at 04:02:06PM -0700, Steven Dake wrote:
> > Unfortunately it appears that del_gendisk uses kobject_del to delete the
> > kobject.  If the kobject has a ktype release function, it is not called
> > in the kobject_del call path, but only in kobject_unregister.
> 
> That's intentional.  gendisks (like everything using kobjects) are
> reference counted and ->release is unly called after the last reference
> goes away, for gendisks that would be the last put_disk call.
> 
> Unless you miss the put_disk call (which md certainly has) there's
> no memeory leak.
> 
Thanks Christoph...

Looks like I'm barking up the wrong tree.  Time to go fix up md :)

-steve
> 

Re: [PATCH} fix defect with kobject memory leaks during del_gendisk

[Steven Dake]

On Thu, 2003-09-25 at 10:26, Patrick Mochel wrote:
> > Attached is a patch which fixes a few memory leaks in 2.6.0-test5. 
> > Comments are welcome as to whether or not this patch is the right
> > solution.
> > 
> > I added the ability to linux MD to generate add and remove hotplug calls
> > on RAID START and STOPs.  To achieve this, I use the del_gendisk call
> > which deletes the gendisk, delete the children kobjects, and delete the
> > parent (in this case, mdX) kobject.
> > 
> > Unfortunately it appears that del_gendisk uses kobject_del to delete the
> > kobject.  If the kobject has a ktype release function, it is not called
> > in the kobject_del call path, but only in kobject_unregister.
> > 
> > This patch changes the functionality so the release function (in this
> > case the block device release function in drivers/block/genhd.c) is
> > called by changing the kobject_del to kobject_unregister.
> 
> It is not the right thing to do, as Christoph pointed out. Gendisks have a 
> lifetime longer than the time between add_disk() and del_gendisk(). They 
> are created before they are added, and the objects may persist longer 
> after they have been removed from view. 
> 
> To delete the last reference, the MD layer should be calling put_disk(), 
> which will trigger release to happen. 
> 
> Your patch will actually cause other bugs, since the final put_disk() of 
> other block drivers will now be called on already freed memory.
> 
> 
> 	Pat
> 
> 
Thanks good call..  Didn't see the put_disk call but it makes sense now.

-steve