[PATCH 2.6.10-rc2-bk15] sysfs_dir_close memory leak

[PATCH 2.6.10-rc2-bk15] sysfs_dir_close memory leak

[Adam J. Richter]

	sysfs_dir_close did not free the "cursor" sysfs_dirent
used for keeping track of position in the list of sysfs_dirent nodes.
Consequently, doing a "find /sys" would leak a sysfs_dirent for
each of the 1140 directories in my /sys tree, or about 36kB
each time.

	This patch was generated against a sysfs tree with a
bunch of other changes, but should apply to the stock Linux tree
with the appropriate line number adjustments.

	If this patch looks OK, I would appreciate it if someone
would forward it downstream for integration.

                    __     ______________
Adam J. Richter        \ /
adam@yggdrasil.com      | g g d r a s i l


diff -u9 linux.prev/fs/sysfs/dir.c linux.noleak/fs/sysfs/dir.c
--- linux.prev/fs/sysfs/dir.c	2004-12-02 14:51:01.000000000 +0800
+++ linux.noleak/fs/sysfs/dir.c	2004-12-03 10:42:06.000000000 +0800
@@ -346,18 +346,21 @@
 static int sysfs_dir_close(struct inode *inode, struct file *file)
 {
 	struct dentry * dentry = file->f_dentry;
 	struct sysfs_dirent * cursor = file->private_data;
 
 	down(&dentry->d_inode->i_sem);
 	list_del_init(&cursor->s_sibling);
 	up(&dentry->d_inode->i_sem);
 
+	BUG_ON(cursor->s_dentry != NULL);
+	release_sysfs_dirent(cursor);
+
 	return 0;
 }
 
 /* Relationship between s_mode and the DT_xxx types */
 static inline unsigned char dt_type(struct sysfs_dirent *sd)
 {
 	return (sd->s_mode >> 12) & 15;
 }
 

Re: [PATCH 2.6.10-rc2-bk15] sysfs_dir_close memory leak

[Chris Wright]

* Adam J. Richter (adam@yggdrasil.com) wrote:
> 	sysfs_dir_close did not free the "cursor" sysfs_dirent
> used for keeping track of position in the list of sysfs_dirent nodes.
> Consequently, doing a "find /sys" would leak a sysfs_dirent for
> each of the 1140 directories in my /sys tree, or about 36kB
> each time.

Yeah, I noticed this as well.  Why the BUGON()?

thanks,
-chris

Re: [PATCH 2.6.10-rc2-bk15] sysfs_dir_close memory leak

[Adam J. Richter]

Chris Wright wrote:
>* Adam J. Richter (adam@yggdrasil.com) wrote:
>> 	sysfs_dir_close did not free the "cursor" sysfs_dirent
>> used for keeping track of position in the list of sysfs_dirent nodes.
>> Consequently, doing a "find /sys" would leak a sysfs_dirent for
>> each of the 1140 directories in my /sys tree, or about 36kB
>> each time.

>Yeah, I noticed this as well.  Why the BUGON()?

	My thinking was that the preconditions in my tree for
calling release_sysfs_dirent are dirent->s_dentry == NULL and
list_empty(&dirent->s_sibling).  The latter should be apparent
from two lines above, but the former is less obvious, although
it is also theoretically always true.

	I'm OK with deleting the BUG_ON().  It was not verifying
anything passed in by an outside caller.

                    __     ______________
Adam J. Richter        \ /
adam@yggdrasil.com      | g g d r a s i l

Re: [PATCH 2.6.10-rc2-bk15] sysfs_dir_close memory leak

[Greg KH]

On Fri, Dec 03, 2004 at 03:05:20AM -0800, Adam J. Richter wrote:
> Chris Wright wrote:
> >* Adam J. Richter (adam@yggdrasil.com) wrote:
> >> 	sysfs_dir_close did not free the "cursor" sysfs_dirent
> >> used for keeping track of position in the list of sysfs_dirent nodes.
> >> Consequently, doing a "find /sys" would leak a sysfs_dirent for
> >> each of the 1140 directories in my /sys tree, or about 36kB
> >> each time.
> 
> >Yeah, I noticed this as well.  Why the BUGON()?
> 
> 	My thinking was that the preconditions in my tree for
> calling release_sysfs_dirent are dirent->s_dentry == NULL and
> list_empty(&dirent->s_sibling).  The latter should be apparent
> from two lines above, but the former is less obvious, although
> it is also theoretically always true.
> 
> 	I'm OK with deleting the BUG_ON().  It was not verifying
> anything passed in by an outside caller.

Thanks, I've deleted the BUG_ON() and will be sending the patch on to
Linus in a bit.

(oh, care to add a "Signed-off-by:" line to your patches?)

thanks,

greg k-h

Re: [PATCH 2.6.10-rc2-bk15] sysfs_dir_close memory leak

[Adam J. Richter]

On Fri, 3 Dec 2004 10:56:31 -0800, Greg KH wrote:
>Thanks, I've deleted the BUG_ON() and will be sending the patch on to
>Linus in a bit.

	Great, thanks.

>(oh, care to add a "Signed-off-by:" line to your patches?)

	When we first started using Signed-off-by: lines, I did,
and then somebody commented on it to me in some way that gave me
the impression that the practice was only for people who "approve"
patches, but I'm happy to resume adding Signed-off-by: lines to all
of my patches that are proposed for integration in the future.

                    __     ______________
Adam J. Richter        \ /
adam@yggdrasil.com      | g g d r a s i l

Re: [PATCH 2.6.10-rc2-bk15] sysfs_dir_close memory leak

[Chris Wright]

* Adam J. Richter (adam@yggdrasil.com) wrote:
> 	When we first started using Signed-off-by: lines, I did,
> and then somebody commented on it to me in some way that gave me
> the impression that the practice was only for people who "approve"
> patches, but I'm happy to resume adding Signed-off-by: lines to all
> of my patches that are proposed for integration in the future.

It's to document the chain.  So you, as patch author, should sign off
as well as those who "approve" it and send it upstream.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net