[PATCH] Bug in do_mount()

[PATCH] Bug in do_mount()

[Jorgen Cederlof]

do_mount() can sometimes fail to mount a filesystem, but still
increment the filesystem module count.

This patch against 2.4.2 should fix the problem.

       Jörgen

--- fs/super.c.orig	Sun Mar 11 20:25:26 2001
+++ fs/super.c	Sun Mar 11 20:05:27 2001
@@ -1414,6 +1414,8 @@
 fail:
 	if (list_empty(&sb->s_mounts))
 		kill_super(sb, 0);
+	else
+		put_filesystem(fstype);
 	goto unlock_out;
 }
 

Re: [PATCH] Bug in do_mount()

[Alexander Viro]

On Sat, 24 Mar 2001, Jorgen Cederlof wrote:

>  	if (list_empty(&sb->s_mounts))
>  		kill_super(sb, 0);
> +	else
> +		put_filesystem(fstype);
>  	goto unlock_out;

That's completely wrong. Reference acquired by get_fs_type() is
released by put_filesystem() (near fs_out), _NOT_ by kill_super().
kill_super() releases the reference stored in ->s_type (created
by get_sb_...()). If superblock stays alive you should not release it.

What bug are you trying to fix?
							Al

Re: [PATCH] Bug in do_mount()

[Jorgen Cederlof]

> >       if (list_empty(&sb->s_mounts))
> >               kill_super(sb, 0);
> > +     else
> > +             put_filesystem(fstype);
> >       goto unlock_out;
 
> Reference acquired by get_fs_type() is
> released by put_filesystem() (near fs_out), _NOT_ by kill_super().

Yes.

> kill_super() releases the reference stored in ->s_type (created
> by get_sb_...()). If superblock stays alive you should not release it.

get_sb_...() will do get_filesystem() even if superblock stays alive.

We get the filesystem twice, in get_fs_type() and get_sb_...(), but if
we goto 'fail:' and don't call kill_super(), we put the filesystem
only once (near fs_out). 

> What bug are you trying to fix?

foofs is a FS_SINGLE filesystem: 
(but the !nd.dentry->d_inode case should be no different)

# grep foofs /proc/modules 
foofs                 6736   0 (unused)
# mount NONE /mnt -t foofs
# grep foofs /proc/modules 
foofs                 6736   1
# mount NONE /mnt -t foofs
mount: NONE already mounted or /mnt busy
mount: according to mtab, NONE is already mounted on /mnt
# grep foofs /proc/modules 
foofs                 6736   2
# mount NONE /mnt -t foofs
mount: NONE already mounted or /mnt busy
mount: according to mtab, NONE is already mounted on /mnt
# grep foofs /proc/modules 
foofs                 6736   3
# umount /mnt
# grep foofs /proc/modules 
foofs                 6736   2
# umount /mnt
umount: /mnt: not mounted
# grep foofs /proc/modules 
foofs                 6736   2
# rmmod foofs
foofs: Device or resource busy

Re: [PATCH] Bug in do_mount()

[Alexander Viro]

On Sat, 24 Mar 2001, Jorgen Cederlof wrote:

> > kill_super() releases the reference stored in ->s_type (created
> > by get_sb_...()). If superblock stays alive you should not release it.
> 
> get_sb_...() will do get_filesystem() even if superblock stays alive.

Sigh... I see what happens, and yes, it's a bug, but fix is wrong.
The real problem is with get_sb_single(), not with do_mount().
get_sb_bdev() and get_sb_nodev() are actually fine and your fix will
break them.

The problem is in the way we keep count for FS_SINGLE - in my current
tree I switched to cleaner variant, but doing the same in 2.4 may be tricky.

_Probably_ the right way to deal with that (for the time being) is add
	if (fstype->fs_flags & FS_SINGLE)
		put_filesystem(fstype);
before the if (list_empty(...))

IOW, 
--- fs/super.c.old Sat Mar 24 12:39:23 2001
+++ fs/super.c     Sat Mar 24 12:39:56 2001
@@ -1412,6 +1412,8 @@
        return retval;
 
 fail:
+       if (fstype->fs_flags & FS_SINGLE)
+               put_filesystem(fstype);
        if (list_empty(&sb->s_mounts))
                kill_super(sb, 0);
        goto unlock_out;

should be OK for now. Once we get clean refcounting for struct super_block
we will be able to get rid of the kludges in that area completely.
								Al