[PATCH] shmem: put inode if alloc_file failed

[PATCH] shmem: put inode if alloc_file failed

[Bob Liu]

Currently if alloc_file failed, inode willn't be put which may cause small
memory leak, this patch fix it.

Signed-off-by: Bob Liu <lliubbo@gmail.com>
---
 mm/shmem.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 7c9cdc6..1e2bea7 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2756,17 +2756,19 @@ struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags
 #ifndef CONFIG_MMU
 	error = ramfs_nommu_expand_for_mapping(inode, size);
 	if (error)
-		goto put_dentry;
+		goto put_inode;
 #endif
 
 	error = -ENFILE;
 	file = alloc_file(&path, FMODE_WRITE | FMODE_READ,
 		  &shmem_file_operations);
 	if (!file)
-		goto put_dentry;
+		goto put_inode;
 
 	return file;
 
+put_inode:
+	iput(inode);
 put_dentry:
 	path_put(&path);
 put_memory:
-- 
1.6.3.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] shmem: using goto to replace several return

[Bob Liu]

Code clean, use goto to replace several return.

Signed-off-by: Bob Liu <lliubbo@gmail.com>
---
 mm/shmem.c |   32 +++++++++++++++-----------------
 1 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 7c9cdc6..99f5915 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1847,17 +1847,13 @@ shmem_mknod(struct inode *dir, struct dentry *dentry, int mode, dev_t dev)
 						     &dentry->d_name, NULL,
 						     NULL, NULL);
 		if (error) {
-			if (error != -EOPNOTSUPP) {
-				iput(inode);
-				return error;
-			}
+			if (error != -EOPNOTSUPP)
+				goto failed_iput;
 		}
 #ifdef CONFIG_TMPFS_POSIX_ACL
 		error = generic_acl_init(inode, dir);
-		if (error) {
-			iput(inode);
-			return error;
-		}
+		if (error)
+			goto failed_iput;
 #else
 		error = 0;
 #endif
@@ -1866,6 +1862,9 @@ shmem_mknod(struct inode *dir, struct dentry *dentry, int mode, dev_t dev)
 		d_instantiate(dentry, inode);
 		dget(dentry); /* Extra count - pin the dentry in core */
 	}
+
+failed_iput:
+	iput(inode);
 	return error;
 }
 
@@ -1987,10 +1986,8 @@ static int shmem_symlink(struct inode *dir, struct dentry *dentry, const char *s
 	error = security_inode_init_security(inode, dir, &dentry->d_name, NULL,
 					     NULL, NULL);
 	if (error) {
-		if (error != -EOPNOTSUPP) {
-			iput(inode);
-			return error;
-		}
+		if (error != -EOPNOTSUPP)
+			goto failed_iput;
 		error = 0;
 	}
 
@@ -2002,10 +1999,8 @@ static int shmem_symlink(struct inode *dir, struct dentry *dentry, const char *s
 		inode->i_op = &shmem_symlink_inline_operations;
 	} else {
 		error = shmem_getpage(inode, 0, &page, SGP_WRITE, NULL);
-		if (error) {
-			iput(inode);
-			return error;
-		}
+		if (error)
+			goto failed_iput;
 		inode->i_mapping->a_ops = &shmem_aops;
 		inode->i_op = &shmem_symlink_inode_operations;
 		kaddr = kmap_atomic(page, KM_USER0);
@@ -2019,7 +2014,10 @@ static int shmem_symlink(struct inode *dir, struct dentry *dentry, const char *s
 	dir->i_ctime = dir->i_mtime = CURRENT_TIME;
 	d_instantiate(dentry, inode);
 	dget(dentry);
-	return 0;
+
+failed_iput:
+	iput(inode);
+	return error;
 }
 
 static void *shmem_follow_link_inline(struct dentry *dentry, struct nameidata *nd)
-- 
1.6.3.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] shmem: put inode if alloc_file failed

[Andrew Morton]

On Tue, 8 Mar 2011 17:14:59 +0800
Bob Liu <lliubbo@gmail.com> wrote:

> Currently if alloc_file failed, inode willn't be put which may cause small
> memory leak, this patch fix it.
> 
> Signed-off-by: Bob Liu <lliubbo@gmail.com>
> ---
>  mm/shmem.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 7c9cdc6..1e2bea7 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2756,17 +2756,19 @@ struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags
>  #ifndef CONFIG_MMU
>  	error = ramfs_nommu_expand_for_mapping(inode, size);
>  	if (error)
> -		goto put_dentry;
> +		goto put_inode;
>  #endif
>  
>  	error = -ENFILE;
>  	file = alloc_file(&path, FMODE_WRITE | FMODE_READ,
>  		  &shmem_file_operations);
>  	if (!file)
> -		goto put_dentry;
> +		goto put_inode;
>  
>  	return file;
>  
> +put_inode:
> +	iput(inode);
>  put_dentry:
>  	path_put(&path);
>  put_memory:

Is this correct?  We've linked the inode to the dentry with
d_instantiate(), so the d_put() will do the iput() on the inode.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] shmem: using goto to replace several return

[Andrew Morton]

On Tue, 8 Mar 2011 17:15:00 +0800
Bob Liu <lliubbo@gmail.com> wrote:

> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1847,17 +1847,13 @@ shmem_mknod(struct inode *dir, struct dentry *dentry, int mode, dev_t dev)
>  						     &dentry->d_name, NULL,
>  						     NULL, NULL);
>  		if (error) {
> -			if (error != -EOPNOTSUPP) {
> -				iput(inode);
> -				return error;
> -			}
> +			if (error != -EOPNOTSUPP)
> +				goto failed_iput;
>  		}
>  #ifdef CONFIG_TMPFS_POSIX_ACL
>  		error = generic_acl_init(inode, dir);
> -		if (error) {
> -			iput(inode);
> -			return error;
> -		}
> +		if (error)
> +			goto failed_iput;
>  #else
>  		error = 0;
>  #endif
> @@ -1866,6 +1862,9 @@ shmem_mknod(struct inode *dir, struct dentry *dentry, int mode, dev_t dev)
>  		d_instantiate(dentry, inode);
>  		dget(dentry); /* Extra count - pin the dentry in core */
>  	}
> +
> +failed_iput:
> +	iput(inode);
>  	return error;
>  }

This adds a big bug: we newly do iput() on the non-error path.

Please, we need much more care and testing than this.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] shmem: put inode if alloc_file failed

[Al Viro]

On Wed, Mar 09, 2011 at 02:58:59PM -0800, Andrew Morton wrote:

> > +put_inode:
> > +	iput(inode);
> >  put_dentry:
> >  	path_put(&path);
> >  put_memory:
> 
> Is this correct?  We've linked the inode to the dentry with
> d_instantiate(), so the d_put() will do the iput() on the inode.


ITYM path_put() and yes, it will.  There's no leak.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] shmem: put inode if alloc_file failed

[Bob Liu]

On Thu, Mar 10, 2011 at 8:03 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Wed, Mar 09, 2011 at 02:58:59PM -0800, Andrew Morton wrote:
>
>> > +put_inode:
>> > +   iput(inode);
>> >  put_dentry:
>> >     path_put(&path);
>> >  put_memory:
>>
>> Is this correct?  We've linked the inode to the dentry with
>> d_instantiate(), so the d_put() will do the iput() on the inode.
>
>
> ITYM path_put() and yes, it will.  There's no leak.
>

Hi,

Yes, I also think path_put() should, But it seems iput() can't free
that memory during NO-MMU arch.

Would you please take a look at thread: [BUG?] shmem: memory leak on
NO-MMU arch ?

http://www.spinics.net/lists/linux-mm/msg15668.html

Thanks
-- 
Regards,
--Bob

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] shmem: put inode if alloc_file failed

[Bob Liu]

On Thu, Mar 10, 2011 at 8:03 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Wed, Mar 09, 2011 at 02:58:59PM -0800, Andrew Morton wrote:
>
>> > +put_inode:
>> > +   iput(inode);
>> >  put_dentry:
>> >     path_put(&path);
>> >  put_memory:
>>
>> Is this correct?  We've linked the inode to the dentry with
>> d_instantiate(), so the d_put() will do the iput() on the inode.
>
>
> ITYM path_put() and yes, it will.  There's no leak.
>

In my opinion, on no-mmu arch iput() will call generate_delete_inode()
> truncate_page_range() which just delete the page from pagecache.

But pages allocated by ramfs_nommu_expand_for_mapping() have
page->_count = 2 (one by alloc_pages + one by pagecache).
The page->_count is still 1 after iput(), so the memory can't be freed.

Is there something I am wrong?

Thanks.

-- 
Regards,
--Bob

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>