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
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 <[email protected]>
--- 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
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.
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 <[email protected]>
--- 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;
}
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 <[email protected]>
> --- 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
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
On Mon, 25 Apr 2005 22:26:26 +0200 (CEST)
Roman Zippel <[email protected]> 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
On Tue, 26 Apr 2005 08:59:14 +0200
Colin Leroy <[email protected]> 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
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?
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