[Patch]Memory leak in sysfs

[Patch]Memory leak in sysfs

[Zou, Nanhai]

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

Some stress testes show there is a memory leak in the latest kernel.
I found the memory leak is in sysfs.
Here is a patch against 2.6.10-rc2-mm4 to fix that.

Signed-off-by: Zou Nan hai <Nanhai.Zou@intel.com>

[-- Attachment #2: sysfs-memleak-fix.patch --]
[-- Type: application/octet-stream, Size: 359 bytes --]

diff -Nraup a/fs/sysfs/dir.c b/fs/sysfs/dir.c
--- a/fs/sysfs/dir.c	2004-12-06 13:04:56.000000000 +0800
+++ b/fs/sysfs/dir.c	2004-12-07 16:49:36.000000000 +0800
@@ -349,6 +349,7 @@ static int sysfs_dir_close(struct inode 
 
 	down(&dentry->d_inode->i_sem);
 	list_del_init(&cursor->s_sibling);
+	sysfs_put(cursor);
 	up(&dentry->d_inode->i_sem);
 
 	return 0;

Re: [Patch]Memory leak in sysfs

[Greg KH]

On Fri, Dec 10, 2004 at 10:21:58AM +0800, Zou, Nanhai wrote:
> Some stress testes show there is a memory leak in the latest kernel.
> I found the memory leak is in sysfs.
> Here is a patch against 2.6.10-rc2-mm4 to fix that.

Is this still needed against the latest -bk tree?  Adam just fixed a
leak like this recently.

And, do you have a pointer to your stress tests?  I'd love to add stuff
like this to an automated testing framework (I know OSDL has one, and
IBM has one internally.)

thanks,

greg k-h

Re: [Patch]Memory leak in sysfs

[Adam J. Richter]

Greg KH wrote:
>And, do you have a pointer to your stress tests?  I'd love to add stuff
>like this to an automated testing framework (I know OSDL has one, and
>IBM has one internally.)

	This is just something that Maneesh had suggested.  My
script runs the commands in separate xterms just so the output
is clearer.
                    __     ______________ 
Adam J. Richter        \ /
adam@yggdrasil.com      | g g d r a s i l


#!/bin/sh

mount -t sysfs x /sys

xterm -geometry 80x10 -title 'cat /sys/class/net/...' \
	-e 'sleep 1 ; while true ; do find /sys/class/net -type f | xargs cat ; done' &

xterm -geometry 80x10 -title '{modprobe,rmmod} dummy' \
	-e 'sleep 1 ; while true ; do modprobe dummy ; rmmod dummy ; done' &

xterm -geometry 80x10 -title 'ls -lR /sys' \
	-e 'sleep 1 ; while true ; do ls -lR /sys ; done' &

wait

Re: [Patch]Memory leak in sysfs

[Adam J. Richter]

Zou, Nanhai wrote:
>Some stress testes show there is a memory leak in the latest kernel.
>I found the memory leak is in sysfs.
>Here is a patch against 2.6.10-rc2-mm4 to fix that.
>
>Signed-off-by: Zou Nan hai <Nanhai.Zou@intel.com>
>
>["sysfs-memleak-fix.patch" (application/octet-stream)]
>
>diff -Nraup a/fs/sysfs/dir.c b/fs/sysfs/dir.c
>--- a/fs/sysfs/dir.c    2004-12-06 13:04:56.000000000 +0800
>+++ b/fs/sysfs/dir.c    2004-12-07 16:49:36.000000000 +0800
>@@ -349,6 +349,7 @@ static int sysfs_dir_close(struct inode 
> 
>        down(&dentry->d_inode->i_sem);
>        list_del_init(&cursor->s_sibling);
>+       sysfs_put(cursor);
>        up(&dentry->d_inode->i_sem);
> 
>        return 0;


	This is will be fixed when the patch that I posted
at http://marc.theaimsgroup.com/?l=linux-kernel&m=110204311025022&w=2
is applied (that patch calls release_sysfs_dirent, since there
is no need for the reference counting of sysfs_put in this case).
I believe that Greg had already indicated he was going to forward
that patch to Linus (without the BUG_ON statement in my patch, which
is fine with me).

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

Re: [PATCH] memory leak in sysfs

[Thomas XB Spatzier]

> It turns out some of the code had changed, to deal with another small bug
> when dentry creation failed. The patch below is updated to -test4.  Could
> you please test it and let me know if it still works?

I've looked over the changes and the code seems to work.


Best regards,

Thomas

[PATCH] memory leak in sysfs

[Thomas Spatzier]

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

Hello Patrick,

please verify the following patch. We found a memory leak in sysfs. Entries
in the dentry_cache allocated for objects in sysfs are not freed when the
objects in sysfs are deleted. This effect is due to inconsistent reference
counting in sysfs. Furthermore, when calling sysfs_remove_dir the deleted
directory was not removed from its parent's list of children. The attached
patch should fix the problems.

(See attached file: linux-2.6.0-test3-sysfs.patch)

Best regards,

Thomas

[-- Attachment #2: linux-2.6.0-test3-sysfs.patch --]
[-- Type: application/octet-stream, Size: 2411 bytes --]

diff -urN linux-2.5/fs/sysfs/bin.c linux-2.5-sysfs-fix/fs/sysfs/bin.c
--- linux-2.5/fs/sysfs/bin.c	Mon Aug 11 18:46:23 2003
+++ linux-2.5-sysfs-fix/fs/sysfs/bin.c	Wed Aug 27 14:57:50 2003
@@ -168,6 +168,7 @@
 			dentry->d_inode->i_size = attr->size;
 			dentry->d_inode->i_fop = &bin_fops;
 		}
+		dput(dentry);
 	} else
 		error = PTR_ERR(dentry);
 	up(&parent->d_inode->i_sem);
diff -urN linux-2.5/fs/sysfs/dir.c linux-2.5-sysfs-fix/fs/sysfs/dir.c
--- linux-2.5/fs/sysfs/dir.c	Fri Jul 11 11:24:41 2003
+++ linux-2.5-sysfs-fix/fs/sysfs/dir.c	Wed Aug 27 14:57:50 2003
@@ -51,6 +51,7 @@
 				     init_dir);
 		if (!error)
 			parent->d_inode->i_nlink++;
+		dput(dentry);
 	} else
 		error = PTR_ERR(dentry);
 	up(&parent->d_inode->i_sem);
@@ -98,6 +99,7 @@
 			 * Unlink and unhash.
 			 */
 			spin_unlock(&dcache_lock);
+			d_delete(d);
 			simple_unlink(dentry->d_inode,d);
 			dput(d);
 			spin_lock(&dcache_lock);
@@ -105,6 +107,7 @@
 		pr_debug(" done\n");
 		node = dentry->d_subdirs.next;
 	}
+	list_del_init(&dentry->d_child);
 	spin_unlock(&dcache_lock);
 	up(&dentry->d_inode->i_sem);
 	d_delete(dentry);
diff -urN linux-2.5/fs/sysfs/file.c linux-2.5-sysfs-fix/fs/sysfs/file.c
--- linux-2.5/fs/sysfs/file.c	Fri Jul 11 11:24:41 2003
+++ linux-2.5-sysfs-fix/fs/sysfs/file.c	Wed Aug 27 14:57:50 2003
@@ -366,6 +366,7 @@
 	if (!IS_ERR(dentry)) {
 		dentry->d_fsdata = (void *)attr;
 		error = sysfs_create(dentry,(attr->mode & S_IALLUGO) | S_IFREG,init_file);
+		dput(dentry);
 	} else
 		error = PTR_ERR(dentry);
 	up(&parent->d_inode->i_sem);
diff -urN linux-2.5/fs/sysfs/inode.c linux-2.5-sysfs-fix/fs/sysfs/inode.c
--- linux-2.5/fs/sysfs/inode.c	Tue Jun 17 13:37:12 2003
+++ linux-2.5-sysfs-fix/fs/sysfs/inode.c	Wed Aug 27 14:57:51 2003
@@ -97,8 +97,8 @@
 			pr_debug("sysfs: Removing %s (%d)\n", victim->d_name.name,
 				 atomic_read(&victim->d_count));
 
-			simple_unlink(dir->d_inode,victim);
 			d_delete(victim);
+			simple_unlink(dir->d_inode,victim);
 		}
 		/*
 		 * Drop reference from sysfs_get_dentry() above.
diff -urN linux-2.5/fs/sysfs/symlink.c linux-2.5-sysfs-fix/fs/sysfs/symlink.c
--- linux-2.5/fs/sysfs/symlink.c	Thu Mar  6 16:01:59 2003
+++ linux-2.5-sysfs-fix/fs/sysfs/symlink.c	Wed Aug 27 14:57:51 2003
@@ -102,6 +102,7 @@
 		error = sysfs_symlink(dentry->d_inode,d,path);
 	else
 		error = PTR_ERR(d);
+	dput(d);
 	up(&dentry->d_inode->i_sem);
 	kfree(path);
 	return error;

Re: [PATCH] memory leak in sysfs

[Patrick Mochel]

> please verify the following patch. We found a memory leak in sysfs. Entries
> in the dentry_cache allocated for objects in sysfs are not freed when the
> objects in sysfs are deleted. This effect is due to inconsistent reference
> counting in sysfs. Furthermore, when calling sysfs_remove_dir the deleted
> directory was not removed from its parent's list of children. The attached
> patch should fix the problems.

Thanks. Martin Schwidefsky had mentioned this to me at OLS (I presume it's 
the same problem). I haven't had a chance to look closely at the patch, 
but it's in my immediate queue..


	Pat

Re: [PATCH] memory leak in sysfs

[Patrick Mochel]

> please verify the following patch. We found a memory leak in sysfs. Entries
> in the dentry_cache allocated for objects in sysfs are not freed when the
> objects in sysfs are deleted. This effect is due to inconsistent reference
> counting in sysfs. Furthermore, when calling sysfs_remove_dir the deleted
> directory was not removed from its parent's list of children. The attached
> patch should fix the problems.

It turns out some of the code had changed, to deal with another small bug
when dentry creation failed. The patch below is updated to -test4.  Could
you please test it and let me know if it still works?

Thanks,


	Pat

===== fs/sysfs/bin.c 1.11 vs edited =====
--- 1.11/fs/sysfs/bin.c	Thu Aug 14 18:17:20 2003
+++ edited/fs/sysfs/bin.c	Thu Aug 28 17:21:05 2003
@@ -168,6 +168,7 @@
 			dentry->d_inode->i_size = attr->size;
 			dentry->d_inode->i_fop = &bin_fops;
 		}
+		dput(dentry);
 	} else
 		error = PTR_ERR(dentry);
 	up(&parent->d_inode->i_sem);
===== fs/sysfs/dir.c 1.9 vs edited =====
--- 1.9/fs/sysfs/dir.c	Wed Aug 13 15:21:11 2003
+++ edited/fs/sysfs/dir.c	Thu Aug 28 17:19:26 2003
@@ -35,10 +35,9 @@
 		if (!error) {
 			dentry->d_fsdata = k;
 			p->d_inode->i_nlink++;
-		} else {
-			dput(dentry);
+		} else
 			dentry = ERR_PTR(error);
-		}
+		dput(dentry);
 	}
 	up(&p->d_inode->i_sem);
 	return dentry;
@@ -136,6 +135,7 @@
 			 * Unlink and unhash.
 			 */
 			spin_unlock(&dcache_lock);
+			d_delete(d);
 			simple_unlink(dentry->d_inode,d);
 			dput(d);
 			spin_lock(&dcache_lock);
@@ -143,6 +143,7 @@
 		pr_debug(" done\n");
 		node = dentry->d_subdirs.next;
 	}
+	list_del_init(&dentry->d_child);
 	spin_unlock(&dcache_lock);
 	up(&dentry->d_inode->i_sem);
 
===== fs/sysfs/file.c 1.10 vs edited =====
--- 1.10/fs/sysfs/file.c	Wed Aug 13 15:21:11 2003
+++ edited/fs/sysfs/file.c	Thu Aug 28 17:17:07 2003
@@ -356,10 +356,9 @@
 		error = sysfs_create(dentry,(attr->mode & S_IALLUGO) | S_IFREG,init_file);
 		if (!error)
 			dentry->d_fsdata = (void *)attr;
-		else {
-			dput(dentry);
+		else
 			dentry = ERR_PTR(error);
-		}
+		dput(dentry);
 	} else
 		error = PTR_ERR(dentry);
 	up(&dir->d_inode->i_sem);
===== fs/sysfs/inode.c 1.86 vs edited =====
--- 1.86/fs/sysfs/inode.c	Thu May 22 16:35:57 2003
+++ edited/fs/sysfs/inode.c	Thu Aug 28 17:17:58 2003
@@ -97,8 +97,8 @@
 			pr_debug("sysfs: Removing %s (%d)\n", victim->d_name.name,
 				 atomic_read(&victim->d_count));
 
-			simple_unlink(dir->d_inode,victim);
 			d_delete(victim);
+			simple_unlink(dir->d_inode,victim);
 		}
 		/*
 		 * Drop reference from sysfs_get_dentry() above.
===== fs/sysfs/symlink.c 1.4 vs edited =====
--- 1.4/fs/sysfs/symlink.c	Tue May 20 00:23:06 2003
+++ edited/fs/sysfs/symlink.c	Thu Aug 28 17:17:46 2003
@@ -102,6 +102,7 @@
 		error = sysfs_symlink(dentry->d_inode,d,path);
 	else
 		error = PTR_ERR(d);
+	dput(d);
 	up(&dentry->d_inode->i_sem);
 	kfree(path);
 	return error;