Skip to contentSkip to conversation

Bug in do_mount()

PATCH In review Jorgen Cederlof 2 participants 4 messages Mar 24, 2001

lore
Share & tools
lore

Jorgen Cederlof

author
#0 lore raw

PATCH 1/1 Bug in do_mount()

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

super.c renamed from super.c.orig +2 -0
@@ -1414,6 +1414,8 @@
 fail:
 	if (list_empty(&sb->s_mounts))
 		kill_super(sb, 0);
+	else
+		put_filesystem(fstype);
 	goto unlock_out;
 }
#1 lore raw

super.c line 1419

Quoted 5 lines · Jorgen Cederlof
super.c renamed from super.c.orig +2 -0
 	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

author
#2 lore raw
In reply to Jorgen Cederlof
Quoted 8 lines

super.c line 1419

Quoted 5 lines
      if (list_empty(&sb->s_mounts))
              kill_super(sb, 0);
super.c renamed from super.c.orig +2 -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.

Quoted 2 lines

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).

Quoted 1 line

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
#3 lore raw
Quoted 4 lines · Jorgen Cederlof
Quoted 2 lines

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,

super.c Sat Mar 24 12:39:56 2001 renamed from super.c.old Sat Mar 24 12:39:23 2001 +2 -0
@@ -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