Messages in this thread | | | Subject | Re: [PATCH] omfs: fix memory leak | From | Davidlohr Bueso <> | Date | Mon, 05 Jul 2010 01:12:39 -0400 |
| |
On Sun, 2010-07-04 at 07:37 -0400, me@bobcopeland.com wrote: > On Sat, Jul 03, 2010 at 10:33:48PM -0400, Davidlohr Bueso wrote: > > 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?
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
> > > > > Signed-off-by: Davidlohr Bueso <dave@gnu.org> > > --- > > fs/omfs/inode.c | 4 +++- > > 1 files changed, 3 insertions(+), 1 deletions(-) > > > > diff --git a/fs/omfs/inode.c b/fs/omfs/inode.c > > index 089839a..253846e 100644 > > --- a/fs/omfs/inode.c > > +++ b/fs/omfs/inode.c > > @@ -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; > > } >
| |