shmem: fix __shmem_file_setup error path leaks

PATCH In review Vito Caputo 3 participants 4 messages Mar 27, 2017 – Mar 28, 2017

Subsystems (2)

Memory management tmpfs (shmem filesystem)

lore

Share & tools

lore

Vito Caputo

author

2017-03-27 17:04 UTC #0 lore raw

PATCH 1/1 shmem: fix __shmem_file_setup error path leaks

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 <[email protected]>

---

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(-)

mm/shmem.c +9 -6

index e67d6ba..a1a84eaf 100644
@@ -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;
 }

Signature · 7 lines

2.1.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Al Viro

2017-03-27 21:21 UTC #1 lore raw

Quoted 7 lines · 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.

Quoted 5 lines

Signed-off-by Vito Caputo <[email protected]>
---
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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

[email protected]

author

2017-03-28 00:51 UTC #2 lore raw

Quoted 11 lines · Al Viro

Quoted 10 lines · 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 <[email protected]>
---

Quoted 8 lines

Quoted 2 lines
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

Hillf Danton

2017-03-28 03:54 UTC #3 lore raw

In reply to Vito Caputo

Quoted 31 lines · 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 <[email protected]>
---
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(-)
mm/shmem.c +1 -1
index e67d6ba..a1a84eaf 100644
@@ -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;

mm/shmem.c static struct file *__shmem_file_setup(const char *name, loff_t size, · line 4168

Quoted 8 lines

mm/shmem.c +1 -1

@@ -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.

Quoted 28 lines

mm/shmem.c +7 -5

@@ -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;
 }
 
--

Quoted 1 line

2.1.4