[PATCH] shmem: fix __shmem_file_setup error path leaks

[PATCH] shmem: fix __shmem_file_setup error path leaks

[Vito Caputo]

The existing path and memory cleanups appear to be in reverse order, and
there's no iput() potentially leaking the inode in the last two error gotos.

Also make put_memory shmem_unacct_size() conditional on !inode since if we
entered cleanup at put_inode, shmem_evict_inode() occurs via
iput()->iput_final(), which performs the shmem_unacct_size() for us.

Signed-off-by: Vito Caputo <vcaputo@pengaru.com>
---

This caught my eye while looking through the memfd_create() implementation.
Included patch was compile tested only...

 mm/shmem.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index e67d6ba..a1a84eaf 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -4134,7 +4134,7 @@ static struct file *__shmem_file_setup(const char *name, loff_t size,
 				       unsigned long flags, unsigned int i_flags)
 {
 	struct file *res;
-	struct inode *inode;
+	struct inode *inode = NULL;
 	struct path path;
 	struct super_block *sb;
 	struct qstr this;
@@ -4162,7 +4162,7 @@ static struct file *__shmem_file_setup(const char *name, loff_t size,
 	res = ERR_PTR(-ENOSPC);
 	inode = shmem_get_inode(sb, NULL, S_IFREG | S_IRWXUGO, 0, flags);
 	if (!inode)
-		goto put_memory;
+		goto put_path;
 
 	inode->i_flags |= i_flags;
 	d_instantiate(path.dentry, inode);
@@ -4170,19 +4170,22 @@ static struct file *__shmem_file_setup(const char *name, loff_t size,
 	clear_nlink(inode);	/* It is unlinked */
 	res = ERR_PTR(ramfs_nommu_expand_for_mapping(inode, size));
 	if (IS_ERR(res))
-		goto put_path;
+		goto put_inode;
 
 	res = alloc_file(&path, FMODE_WRITE | FMODE_READ,
 		  &shmem_file_operations);
 	if (IS_ERR(res))
-		goto put_path;
+		goto put_inode;
 
 	return res;
 
-put_memory:
-	shmem_unacct_size(flags, size);
+put_inode:
+	iput(inode);
 put_path:
 	path_put(&path);
+put_memory:
+	if (!inode)
+		shmem_unacct_size(flags, size);
 	return res;
 }
 
-- 
2.1.4

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] shmem: fix __shmem_file_setup error path leaks

[Al Viro]

On Mon, Mar 27, 2017 at 10:05:34AM -0700, Vito Caputo wrote:
> The existing path and memory cleanups appear to be in reverse order, and
> there's no iput() potentially leaking the inode in the last two error gotos.
> 
> Also make put_memory shmem_unacct_size() conditional on !inode since if we
> entered cleanup at put_inode, shmem_evict_inode() occurs via
> iput()->iput_final(), which performs the shmem_unacct_size() for us.
> 
> Signed-off-by: Vito Caputo <vcaputo@pengaru.com>
> ---
> 
> This caught my eye while looking through the memfd_create() implementation.
> Included patch was compile tested only...

Obviously so, since you've just introduced a double iput() there.  After
        d_instantiate(path.dentry, inode);
dropping the reference to path.dentry (done by path_put(&path)) will drop
the reference to inode transferred into that dentry by d_instantiate().
NAK.

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] shmem: fix __shmem_file_setup error path leaks

[vcaputo]

On Mon, Mar 27, 2017 at 10:21:27PM +0100, Al Viro wrote:
> On Mon, Mar 27, 2017 at 10:05:34AM -0700, Vito Caputo wrote:
> > The existing path and memory cleanups appear to be in reverse order, and
> > there's no iput() potentially leaking the inode in the last two error gotos.
> > 
> > Also make put_memory shmem_unacct_size() conditional on !inode since if we
> > entered cleanup at put_inode, shmem_evict_inode() occurs via
> > iput()->iput_final(), which performs the shmem_unacct_size() for us.
> > 
> > Signed-off-by: Vito Caputo <vcaputo@pengaru.com>
> > ---
> > 
> > This caught my eye while looking through the memfd_create() implementation.
> > Included patch was compile tested only...
> 
> Obviously so, since you've just introduced a double iput() there.  After
>         d_instantiate(path.dentry, inode);
> dropping the reference to path.dentry (done by path_put(&path)) will drop
> the reference to inode transferred into that dentry by d_instantiate().
> NAK.

I see, so it's correct as-is, thanks for the review and apologies for the
noise!

Cheers,
Vito Caputo

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] shmem: fix __shmem_file_setup error path leaks

[Hillf Danton]

On March 28, 2017 1:06 AM Vito Caputo wrote:
> 
> The existing path and memory cleanups appear to be in reverse order, and
> there's no iput() potentially leaking the inode in the last two error gotos.
> 
> Also make put_memory shmem_unacct_size() conditional on !inode since if we
> entered cleanup at put_inode, shmem_evict_inode() occurs via
> iput()->iput_final(), which performs the shmem_unacct_size() for us.
> 
> Signed-off-by: Vito Caputo <vcaputo@pengaru.com>
> ---
> 
> This caught my eye while looking through the memfd_create() implementation.
> Included patch was compile tested only...
> 
>  mm/shmem.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index e67d6ba..a1a84eaf 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -4134,7 +4134,7 @@ static struct file *__shmem_file_setup(const char *name, loff_t size,
>  				       unsigned long flags, unsigned int i_flags)
>  {
>  	struct file *res;
> -	struct inode *inode;
> +	struct inode *inode = NULL;
>  	struct path path;
>  	struct super_block *sb;
>  	struct qstr this;
> @@ -4162,7 +4162,7 @@ static struct file *__shmem_file_setup(const char *name, loff_t size,
>  	res = ERR_PTR(-ENOSPC);
>  	inode = shmem_get_inode(sb, NULL, S_IFREG | S_IRWXUGO, 0, flags);
>  	if (!inode)
> -		goto put_memory;
> +		goto put_path;
> 
>  	inode->i_flags |= i_flags;
>  	d_instantiate(path.dentry, inode);

After this routine, the inode is in use of the dcache, as its comment says.

> @@ -4170,19 +4170,22 @@ static struct file *__shmem_file_setup(const char *name, loff_t size,
>  	clear_nlink(inode);	/* It is unlinked */
>  	res = ERR_PTR(ramfs_nommu_expand_for_mapping(inode, size));
>  	if (IS_ERR(res))
> -		goto put_path;
> +		goto put_inode;
> 
>  	res = alloc_file(&path, FMODE_WRITE | FMODE_READ,
>  		  &shmem_file_operations);
>  	if (IS_ERR(res))
> -		goto put_path;
> +		goto put_inode;
> 
>  	return res;
> 
> -put_memory:
> -	shmem_unacct_size(flags, size);
> +put_inode:
> +	iput(inode);
>  put_path:
>  	path_put(&path);
> +put_memory:
> +	if (!inode)
> +		shmem_unacct_size(flags, size);
>  	return res;
>  }
> 
> --
> 2.1.4

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>