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(-)
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
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 <[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.
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 <[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.
I see, so it's correct as-is, thanks for the review and apologies for the
noise!
Cheers,
Vito Caputo
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 <[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(-)
>
> 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