2010-04-12 16:40:24

by Jan Blunck

Subject: [PATCH] JFS: Free sbi memory in error path

I spotted the missing kfree() while removing the BKL.

Signed-off-by: Jan Blunck <[email protected]>
---
fs/jfs/super.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/jfs/super.c b/fs/jfs/super.c
index 266699d..a402b7d 100644
--- a/fs/jfs/super.c
+++ b/fs/jfs/super.c
@@ -457,6 +457,7 @@ static int jfs_fill_super(struct super_block *sb, void *data, int silent)

if (newLVSize) {
printk(KERN_ERR "resize option for remount only\n");
+ kfree(sbi);
return -EINVAL;
}

--
1.6.4.2

2010-04-12 23:44:22

by Andrew Morton

Subject: Re: [PATCH] JFS: Free sbi memory in error path

On Mon, 12 Apr 2010 18:40:17 +0200
Jan Blunck <[email protected]> wrote:

> I spotted the missing kfree() while removing the BKL.
>
> Signed-off-by: Jan Blunck <[email protected]>
> ---
> fs/jfs/super.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/fs/jfs/super.c b/fs/jfs/super.c
> index 266699d..a402b7d 100644
> --- a/fs/jfs/super.c
> +++ b/fs/jfs/super.c
> @@ -457,6 +457,7 @@ static int jfs_fill_super(struct super_block *sb, void *data, int silent)
>
> if (newLVSize) {
> printk(KERN_ERR "resize option for remount only\n");
> + kfree(sbi);
> return -EINVAL;
> }

It'd be best to remove those multiple return statements so that leaks
of this nature are less likely to be reintroduced:


From: Jan Blunck <[email protected]>

I spotted the missing kfree() while removing the BKL.

[[email protected]: avoid multiple returns so it doesn't happen again]
Signed-off-by: Jan Blunck <[email protected]>
Cc: Dave Kleikamp <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---

fs/jfs/super.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)

diff -puN fs/jfs/super.c~jfs-free-sbi-memory-in-error-path fs/jfs/super.c
--- a/fs/jfs/super.c~jfs-free-sbi-memory-in-error-path
+++ a/fs/jfs/super.c
@@ -446,10 +446,8 @@ static int jfs_fill_super(struct super_b
/* initialize the mount flag and determine the default error handler */
flag = JFS_ERR_REMOUNT_RO;

- if (!parse_options((char *) data, sb, &newLVSize, &flag)) {
- kfree(sbi);
- return -EINVAL;
- }
+ if (!parse_options((char *) data, sb, &newLVSize, &flag))
+ goto out_kfree;
sbi->flag = flag;

#ifdef CONFIG_JFS_POSIX_ACL
@@ -458,7 +456,7 @@ static int jfs_fill_super(struct super_b

if (newLVSize) {
printk(KERN_ERR "resize option for remount only\n");
- return -EINVAL;
+ goto out_kfree;
}

/*
@@ -478,7 +476,7 @@ static int jfs_fill_super(struct super_b
inode = new_inode(sb);
if (inode == NULL) {
ret = -ENOMEM;
- goto out_kfree;
+ goto out_unload;
}
inode->i_ino = 0;
inode->i_nlink = 1;
@@ -550,9 +548,10 @@ out_mount_failed:
make_bad_inode(sbi->direct_inode);
iput(sbi->direct_inode);
sbi->direct_inode = NULL;
-out_kfree:
+out_unload:
if (sbi->nls_tab)
unload_nls(sbi->nls_tab);
+out_kfree:
kfree(sbi);
return ret;
}
_

2010-04-13 07:58:37

by Jan Blunck

Subject: Re: [PATCH] JFS: Free sbi memory in error path

On Mon, Apr 12, Andrew Morton wrote:

> On Mon, 12 Apr 2010 18:40:17 +0200
> Jan Blunck <[email protected]> wrote:
>
> > I spotted the missing kfree() while removing the BKL.
> >
> > Signed-off-by: Jan Blunck <[email protected]>
> > ---
> > fs/jfs/super.c | 1 +
> > 1 files changed, 1 insertions(+), 0 deletions(-)
> >
> > diff --git a/fs/jfs/super.c b/fs/jfs/super.c
> > index 266699d..a402b7d 100644
> > --- a/fs/jfs/super.c
> > +++ b/fs/jfs/super.c
> > @@ -457,6 +457,7 @@ static int jfs_fill_super(struct super_block *sb, void *data, int silent)
> >
> > if (newLVSize) {
> > printk(KERN_ERR "resize option for remount only\n");
> > + kfree(sbi);
> > return -EINVAL;
> > }
>
> It'd be best to remove those multiple return statements so that leaks
> of this nature are less likely to be reintroduced:
>

Of course. Thanks Andrew.

Jan

>
> From: Jan Blunck <[email protected]>
>
> I spotted the missing kfree() while removing the BKL.
>
> [[email protected]: avoid multiple returns so it doesn't happen again]
> Signed-off-by: Jan Blunck <[email protected]>
> Cc: Dave Kleikamp <[email protected]>
> Signed-off-by: Andrew Morton <[email protected]>
> ---
>
> fs/jfs/super.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff -puN fs/jfs/super.c~jfs-free-sbi-memory-in-error-path fs/jfs/super.c
> --- a/fs/jfs/super.c~jfs-free-sbi-memory-in-error-path
> +++ a/fs/jfs/super.c
> @@ -446,10 +446,8 @@ static int jfs_fill_super(struct super_b
> /* initialize the mount flag and determine the default error handler */
> flag = JFS_ERR_REMOUNT_RO;
>
> - if (!parse_options((char *) data, sb, &newLVSize, &flag)) {
> - kfree(sbi);
> - return -EINVAL;
> - }
> + if (!parse_options((char *) data, sb, &newLVSize, &flag))
> + goto out_kfree;
> sbi->flag = flag;
>
> #ifdef CONFIG_JFS_POSIX_ACL
> @@ -458,7 +456,7 @@ static int jfs_fill_super(struct super_b
>
> if (newLVSize) {
> printk(KERN_ERR "resize option for remount only\n");
> - return -EINVAL;
> + goto out_kfree;
> }
>
> /*
> @@ -478,7 +476,7 @@ static int jfs_fill_super(struct super_b
> inode = new_inode(sb);
> if (inode == NULL) {
> ret = -ENOMEM;
> - goto out_kfree;
> + goto out_unload;
> }
> inode->i_ino = 0;
> inode->i_nlink = 1;
> @@ -550,9 +548,10 @@ out_mount_failed:
> make_bad_inode(sbi->direct_inode);
> iput(sbi->direct_inode);
> sbi->direct_inode = NULL;
> -out_kfree:
> +out_unload:
> if (sbi->nls_tab)
> unload_nls(sbi->nls_tab);
> +out_kfree:
> kfree(sbi);
> return ret;
> }
> _
>
Regards,
Jan

--
Jan Blunck <[email protected]>