Re: [PATCH] hfsplus: don't oops on bad FS

Re: [PATCH] hfsplus: don't oops on bad FS

[Roman Zippel]

Hi,

On Mon, 25 Apr 2005, Colin Leroy wrote:

> currently trying to mount a non-hfsplus filesystem as hfsplus results
> in an oops, as seen on http://colino.net/tmp/hfsplus_oops.txt
> 
> This patch fixes it; while at it, it frees sbi on error instead of
> leaking it.

Actually it looks like we are always leaking it, so actually 
hfsplus_put_super() needs fixing, could you add the check and kfree 
there and do the same fix for hfs?

bye, Roman

Re: [PATCH] hfsplus: don't oops on bad FS

[Colin Leroy]

On 25 Apr 2005 at 21h04, Roman Zippel wrote:

Hi, 

> Actually it looks like we are always leaking it, so actually 
> hfsplus_put_super() needs fixing, could you add the check and kfree 
> there and do the same fix for hfs?

Mmh, right. Here's an updated version that fixes it too.

Signed-off-by: Colin Leroy <colin@colino.net>
--- a/fs/hfsplus/super.c	2005-04-25 21:56:56.000000000 +0200
+++ b/fs/hfsplus/super.c	2005-04-25 21:58:39.000000000 +0200
@@ -226,6 +226,9 @@
 	brelse(HFSPLUS_SB(sb).s_vhbh);
 	if (HFSPLUS_SB(sb).nls)
 		unload_nls(HFSPLUS_SB(sb).nls);
+
+	kfree((struct hfsplus_sb_info *)sb->s_fs_info);
+	sb->s_fs_info = NULL;
 }
 
 static int hfsplus_statfs(struct super_block *sb, struct kstatfs *buf)
@@ -298,7 +301,7 @@
 		if (!silent)
 			printk("HFS+-fs: unable to parse mount options\n");
 		err = -EINVAL;
-		goto cleanup;
+		goto cleanup_little;
 	}
 
 	/* temporarily use utf8 to correctly find the hidden dir below */
@@ -307,7 +310,7 @@
 	if (!nls) {
 		printk("HFS+: unable to load nls for utf8\n");
 		err = -EINVAL;
-		goto cleanup;
+		goto cleanup_little;
 	}
 
 	/* Grab the volume header */
@@ -315,7 +318,7 @@
 		if (!silent)
 			printk("HFS+-fs: unable to find HFS+ superblock\n");
 		err = -EINVAL;
-		goto cleanup;
+		goto cleanup_little;
 	}
 	vhdr = HFSPLUS_SB(sb).s_vhdr;
 
@@ -428,8 +431,12 @@
 
 cleanup:
 	hfsplus_put_super(sb);
+
+cleanup_little:
 	if (nls)
 		unload_nls(nls);
+	sb->s_fs_info = NULL;
+	kfree(sbi);
 	return err;
 }
 

-- 
Colin

Re: [PATCH] hfsplus: don't oops on bad FS

[Christoph Hellwig]

On Mon, Apr 25, 2005 at 10:03:45PM +0200, Colin Leroy wrote:
> +	kfree((struct hfsplus_sb_info *)sb->s_fs_info);

absolutely no need to cast here.

Re: [PATCH] hfsplus: don't oops on bad FS

[Colin Leroy]

On 25 Apr 2005 at 21h04, Christoph Hellwig wrote:

Hi, 

> On Mon, Apr 25, 2005 at 10:03:45PM +0200, Colin Leroy wrote:
> 
> absolutely no need to cast here.

Third update then :-)

Signed-off-by: Colin Leroy <colin@colino.net>
--- a/fs/hfsplus/super.c	2005-04-25 21:56:56.000000000 +0200
+++ b/fs/hfsplus/super.c	2005-04-25 21:58:39.000000000 +0200
@@ -226,6 +226,9 @@
 	brelse(HFSPLUS_SB(sb).s_vhbh);
 	if (HFSPLUS_SB(sb).nls)
 		unload_nls(HFSPLUS_SB(sb).nls);
+
+	kfree(sb->s_fs_info);
+	sb->s_fs_info = NULL;
 }
 
 static int hfsplus_statfs(struct super_block *sb, struct kstatfs *buf)
@@ -298,7 +301,7 @@
 		if (!silent)
 			printk("HFS+-fs: unable to parse mount options\n");
 		err = -EINVAL;
-		goto cleanup;
+		goto cleanup_little;
 	}
 
 	/* temporarily use utf8 to correctly find the hidden dir below */
@@ -307,7 +310,7 @@
 	if (!nls) {
 		printk("HFS+: unable to load nls for utf8\n");
 		err = -EINVAL;
-		goto cleanup;
+		goto cleanup_little;
 	}
 
 	/* Grab the volume header */
@@ -315,7 +318,7 @@
 		if (!silent)
 			printk("HFS+-fs: unable to find HFS+ superblock\n");
 		err = -EINVAL;
-		goto cleanup;
+		goto cleanup_little;
 	}
 	vhdr = HFSPLUS_SB(sb).s_vhdr;
 
@@ -428,8 +431,12 @@
 
 cleanup:
 	hfsplus_put_super(sb);
+
+cleanup_little:
 	if (nls)
 		unload_nls(nls);
+	sb->s_fs_info = NULL;
+	kfree(sbi);
 	return err;
 }
 

Re: [PATCH] hfsplus: don't oops on bad FS

[Roman Zippel]

Hi,

On Mon, 25 Apr 2005, Colin Leroy wrote:

> > Actually it looks like we are always leaking it, so actually 
> > hfsplus_put_super() needs fixing, could you add the check and kfree 
> > there and do the same fix for hfs?
> 
> Mmh, right. Here's an updated version that fixes it too.

Don't change hfsplus_fill_super, add a "if (!sb->s_fs_info) return;" to 
hfsplus_put_super (and also hfs_put_super, so you can kill kfree from 
hfs_fill_super).

bye, Roman

Re: [PATCH] hfsplus: don't oops on bad FS

[Jesper Juhl]

On Mon, 25 Apr 2005, Colin Leroy wrote:

> On 25 Apr 2005 at 21h04, Roman Zippel wrote:
> 
> Hi, 
> 
> > Actually it looks like we are always leaking it, so actually 
> > hfsplus_put_super() needs fixing, could you add the check and kfree 
> > there and do the same fix for hfs?
> 
> Mmh, right. Here's an updated version that fixes it too.
> 
> Signed-off-by: Colin Leroy <colin@colino.net>
> --- a/fs/hfsplus/super.c	2005-04-25 21:56:56.000000000 +0200
> +++ b/fs/hfsplus/super.c	2005-04-25 21:58:39.000000000 +0200
> @@ -226,6 +226,9 @@
>  	brelse(HFSPLUS_SB(sb).s_vhbh);
>  	if (HFSPLUS_SB(sb).nls)
>  		unload_nls(HFSPLUS_SB(sb).nls);
> +
> +	kfree((struct hfsplus_sb_info *)sb->s_fs_info);

kfree() takes a  void *  argument, that cast is not needed.


-- 
Jesper Juhl

Re: [PATCH] hfsplus: don't oops on bad FS

[Colin Leroy]

On Mon, 25 Apr 2005 22:26:26 +0200 (CEST)
Roman Zippel <zippel@linux-m68k.org> wrote:

Hi,

> > > Actually it looks like we are always leaking it, so actually 
> > > hfsplus_put_super() needs fixing, could you add the check and
> > > kfree there and do the same fix for hfs?
> > 
> > Mmh, right. Here's an updated version that fixes it too.
> 
> Don't change hfsplus_fill_super, add a "if (!sb->s_fs_info) return;"
> to hfsplus_put_super 

I don't get it. I do have to change hfsplus_fill_super to free 
sb->s_fs_info, don't I? If I just nullify it there, there will be a
leak, and if I don't, put_super won't know it shouldn't do anything.

This try brings in less changes, but I don't see how to do even less:
(pseudo-patch that won't apply, just to know):
--- fs/hfsplus/super.c.orig     2005-04-25 21:56:56.000000000 +0200
+++ fs/hfsplus/super.c  2005-04-26 08:57:22.000000000 +0200
@@ -207,6 +207,9 @@ static void hfsplus_write_super(struct s

 static void hfsplus_put_super(struct super_block *sb)
 {
+       if (!sb->s_fs_info)
+               return;
+
        dprint(DBG_SUPER, "hfsplus_put_super\n");
        if (!(sb->s_flags & MS_RDONLY)) {
                struct hfsplus_vh *vhdr = HFSPLUS_SB(sb).s_vhdr;
@@ -226,6 +229,9 @@ static void hfsplus_put_super(struct sup
        brelse(HFSPLUS_SB(sb).s_vhbh);
        if (HFSPLUS_SB(sb).nls)
                unload_nls(HFSPLUS_SB(sb).nls);
+
+       kfree(sb->s_fs_info);
+       sb->s_fs_info = NULL;
 }

 static int hfsplus_statfs(struct super_block *sb, struct kstatfs *buf)
@@ -427,6 +433,9 @@ out:
        return 0;

 cleanup:
+       kfree(sb->s_fs_info);
+       sb->s_fs_info = NULL;
+
        hfsplus_put_super(sb);
        if (nls)
                unload_nls(nls);

What do you think?

Thanks,
-- 
Colin

Re: [PATCH] hfsplus: don't oops on bad FS

[Colin Leroy]

On Tue, 26 Apr 2005 08:59:14 +0200
Colin Leroy <colin@colino.net> wrote:

>  cleanup:
> +       kfree(sb->s_fs_info);
> +       sb->s_fs_info = NULL;
> +

Also, that may be wrong: maybe hfsplus_put_super has a job to do if
mounting fails later than "no hfs+ fs found".

My understanding of the driver is limited, that's why my initial patch
did the less possible functionality change. But I'd like to remember
you (maybe you forgot) that my initial patch wasn't about fixing the
s_fs_info leak, but rather fixing an oops that happens in
hfsplus_put_super. That's why I don't think we can run the current code
in hfsplus_put_super from hfsplus_fill_super cleanup part : 

HFS+-fs: unable to find HFS+ superblock
Oops: kernel access of bad area, sig: 11 [#1]
NIP: EA4707F4 LR: EA470AC8 SP: CC91DAA0 REGS: cc91d9f0 TRAP: 0300    Not tainted
MSR: 00009032 EE: 1 PR: 0 FP: 0 ME: 1 IR/DR: 11
DAR: 00000004, DSISR: 40000000
TASK = ce48cdf0[20295] 'mount' THREAD: cc91c000
Last syscall: 21
GPR00: 00000000 CC91DAA0 CE48CDF0 CB2FF200 C0373ECC 00000004 E756CD60 3B9ACA00
GPR08: C2B71F60 C0360000 00000000 BE932A74 0000D903 1002957C 10020000 10026810
GPR16: 100267E0 10026840 7FF3F4DD 100267D0 7FF3F4B3 00000000 10026820 10026820
GPR24: 7FF3EF70 EA4709EC EA105714 00000000 00000000 C9341000 C2B71F60 CB2FF200
NIP [ea4707f4] hfsplus_put_super+0x9c/0x114 [hfsplus]
LR [ea470ac8] hfsplus_fill_super+0xdc/0x5a8 [hfsplus]
Call trace:
 [ea470ac8] hfsplus_fill_super+0xdc/0x5a8 [hfsplus]
 [c00644e4] get_sb_bdev+0x14c/0x1d4
 [ea471018] hfsplus_get_sb+0x18/0x28 [hfsplus]
 [c0064824] do_kern_mount+0x5c/0x130
 [c007c774] do_mount+0x46c/0x6cc
 [c007ce18] sys_mount+0x98/0xe8
 [c0004840] ret_from_syscall+0x0/0x44

-- 
Colin

Re: [PATCH] hfsplus: don't oops on bad FS

[Chris Wedgwood]

On Mon, Apr 25, 2005 at 10:03:45PM +0200, Colin Leroy wrote:

>  cleanup:
>  	hfsplus_put_super(sb);
> +
> +cleanup_little:
>  	if (nls)
>  		unload_nls(nls);
> +	sb->s_fs_info = NULL;
> +	kfree(sbi);

cleanup_little?  why not cleanup_no_put or something?

Re: [PATCH] hfsplus: don't oops on bad FS

[Colin Leroy]

On 30 Apr 2005 at 22h04, Chris Wedgwood wrote:

Hi, 

> > +cleanup_little:
> >  	if (nls)
> >  		unload_nls(nls);
> > +	sb->s_fs_info = NULL;
> > +	kfree(sbi);
> 
> cleanup_little?  why not cleanup_no_put or something?

I was lacking this kind of inspiration :)
Roman's patch that Andrew just pushed superceded this one, anyway.

-- 
Colin