omfs: fix memory leak

PATCH In review Davidlohr Bueso 2 participants 6 messages Jul 4, 2010 – Jul 6, 2010

Subsystems (2)

VFS Omfs filesystem

lore

Share & tools

lore

Davidlohr Bueso

author

2010-07-04 02:33 UTC #0 lore raw

PATCH 1/1 omfs: fix memory leak

Hi,

In omfs_fill_super(), when returning on error, sbi is not being freed.

Thanks,
Davidlohr.

Signed-off-by Davidlohr Bueso <[email protected]>

---
 fs/omfs/inode.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

fs/omfs/inode.c +3 -1

index 089839a..253846e 100644
@@ -523,12 +523,14 @@ static int omfs_fill_super(struct super_block *sb, void *data, int silent)
 	}
 	printk(KERN_DEBUG "omfs: Mounted volume %s\n", omfs_rb->r_name);
 
-	ret = 0;
+	ret = 0; /* success */
 out_brelse_bh2:
 	brelse(bh2);
 out_brelse_bh:
 	brelse(bh);
 end:
+	if (ret != 0)
+		kfree(sbi);
 	return ret;
 }

Signature · 9 lines

1.7.0.4




------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone?
Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first

[email protected]

maintainer

2010-07-04 11:40 UTC #1 lore raw

Quoted 6 lines · Davidlohr Bueso

Hi,
In omfs_fill_super(), when returning on error, sbi is not being freed.
Thanks,
Davidlohr.

Hi Davidlohr,

I don't think this is right:

fill_super:
err = omfs_fill_super()
if (err)

     deactivate_locked_super(sb)
        kill_sb()
          generic_shutdown_super()
            sop->put_super()
  ...
  omfs_put_super()
    kfree(sbi->s_imap);
    kfree(sbi);

So your change would cause a crash at the first kfree in omfs_put_super().

It looks fine to me as-is, or am I missing something?

Quoted 25 lines

Signed-off-by Davidlohr Bueso <[email protected]>

---
 fs/omfs/inode.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

fs/omfs/inode.c +3 -1

index 089839a..253846e 100644
@@ -523,12 +523,14 @@ static int omfs_fill_super(struct super_block *sb, void *data, int silent)
 	}
 	printk(KERN_DEBUG "omfs: Mounted volume %s\n", omfs_rb->r_name);
 
-	ret = 0;
+	ret = 0; /* success */
 out_brelse_bh2:
 	brelse(bh2);
 out_brelse_bh:
 	brelse(bh);
 end:
+	if (ret != 0)
+		kfree(sbi);
 	return ret;
 }

Signature · 1 line

Bob Copeland %% www.bobcopeland.com

Davidlohr Bueso

author

2010-07-05 05:12 UTC #2 lore raw

Quoted 24 lines · On Sun, 2010-07-04 at 07:37 -0400, [email protected] wrote:

Quoted 6 lines · Davidlohr Bueso
Hi,
In omfs_fill_super(), when returning on error, sbi is not being freed.
Thanks,
Davidlohr.
Hi Davidlohr,
I don't think this is right:
fill_super:
err = omfs_fill_super()
if (err)
     deactivate_locked_super(sb)
        kill_sb()
          generic_shutdown_super()
            sop->put_super()
  ...
  omfs_put_super()
    kfree(sbi->s_imap);
    kfree(sbi);

Quoted 3 lines

So your change would cause a crash at the first kfree in omfs_put_super().
It looks fine to me as-is, or am I missing something?

Thanks for the reply, Bob. Please bare with me, as I am learning about the VFS workings. To my knowledge this would happen:

a user tries to mount a omfs partition:

	omfs_get_sb() -> omfs_fill_super()

If omfs_fill_super() fails, in this case 'ret' being a non 0 value, the mount will not be completed, hence the previously allocated 'sbi' variable will not be freed.

Isn't put_super() called to free data when things run "normally", like for unmounting? So this function does two things:

kfree(sbi->s_imap)
kfree(sbi)

However, in omfs_get_imap() 'sbi->s_imap' is freed upon failure, so wouldn't this also crash on the first kfree in omfs_put_super()? Wouldn't this be the same for freeing sbi upon failure in omfs_fill_super()?

Now, the patch I sent is compile-tested only, so it might crash it. I will test it properly.

If what I just said is complete nonsense, please enlighten me.

Thanks,
Davidlohr

Quoted 15 lines

Signed-off-by Davidlohr Bueso <[email protected]>

---
 fs/omfs/inode.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

fs/omfs/inode.c +0 -0

index 089839a..253846e 100644
@@ -523,12 +523,14 @@ static int omfs_fill_super(struct super_block *sb, void *data, int silent)
 	}
 	printk(KERN_DEBUG "omfs: Mounted volume %s\n", omfs_rb->r_name);

Quoted 12 lines

fs/omfs/inode.c static int omfs_fill_super(struct super_block *sb, void *data, int silent) · line 535
Quoted 11 lines
fs/omfs/inode.c +3 -1
-	ret = 0;
+	ret = 0; /* success */
 out_brelse_bh2:
 	brelse(bh2);
 out_brelse_bh:
 	brelse(bh);
 end:
+	if (ret != 0)
+		kfree(sbi);
 	return ret;
 }

------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone? Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first

[email protected]

maintainer

2010-07-05 14:00 UTC #3 lore raw

Quoted 2 lines · Davidlohr Bueso

Isn't put_super() called to free data when things run "normally", like for unmounting? So this function does two things:

Ok, I checked it out and you are right, FS put_super is only called after successful mount so there is a leak. I'll take your patch, but please:

- remove the /* success */ comment, IMO it's just noise
- write the if conditional as the more usual:

if (ret)

Quoted 5 lines

kfree(sbi->s_imap)
kfree(sbi)
However, in omfs_get_imap() 'sbi->s_imap' is freed upon failure, so wouldn't this also crash on the first kfree in omfs_put_super()?

This is ok, since sbi->s_imap is set to null in that case and kfree(NULL) is fine.

Thanks for the review!

Signature · 7 lines

Bob Copeland %% www.bobcopeland.com


------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone?
Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first

Davidlohr Bueso

author

2010-07-06 04:50 UTC #4 lore raw

Quoted 4 lines · On Mon, 2010-07-05 at 10:00 -0400, [email protected] wrote:

Quoted 2 lines · Davidlohr Bueso
Isn't put_super() called to free data when things run "normally", like for unmounting? So this function does two things:

Quoted 9 lines

Ok, I checked it out and you are right, FS put_super is only called after successful mount so there is a leak. I'll take your patch, but please:
- remove the /* success */ comment, IMO it's just noise
- write the if conditional as the more usual:
if (ret)

Ok, resending that patch with the corrections. Thanks.

Signed-off-by Davidlohr Bueso <[email protected]>

---
 fs/omfs/inode.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

fs/omfs/inode.c +2 -0

index 089839a..b5d6380 100644
@@ -529,6 +529,8 @@ out_brelse_bh2:
 out_brelse_bh:
 	brelse(bh);
 end:
+	if (ret)
+		kfree(sbi);
 	return ret;
 }

Signature · 1 line

1.7.0.4

Quoted 11 lines

Quoted 5 lines
kfree(sbi->s_imap)
kfree(sbi)
However, in omfs_get_imap() 'sbi->s_imap' is freed upon failure, so wouldn't this also crash on the first kfree in omfs_put_super()?
This is ok, since sbi->s_imap is set to null in that case and kfree(NULL) is fine.
Thanks for the review!

[email protected]

maintainer

2010-07-06 15:45 UTC #5 lore raw

Quoted 3 lines · Davidlohr Bueso

Ok, resending that patch with the corrections. Thanks.
Signed-off-by Davidlohr Bueso <[email protected]>

Great, applied to the fixes branch of

git://git.kernel.org/pub/scm/linux/kernel/git/bcopeland/omfs.git

Thanks!

Signature · 7 lines

Bob Copeland %% www.bobcopeland.com


------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone?
Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first