[PATCH] vxfs fix

[PATCH] vxfs fix

[Andries.Brouwer]

Dear Linus, Alan, Al, Christoph, all,

If one mounts without specifying a type, mount will try
all available types. After having tried vxfs the next type
will cause
	set_blocksize: b_count 1 ...
since vxfs forgets to free a block.
The patch below adds the missing brelse().
(In fact there are more resources that are never freed there -
maybe the maintainer can have a look some time -
I only added a comment.)

When mount continues to try all types, it may try V7.
That always succeeds, there is no test for magic or so,
and after garbage has been mounted as a V7 filesystem,
the kernel crashes or hangs or fails in other sad ways.
Have not tried to debug.

Andries


--- ../linux-2.4.7/linux/fs/freevxfs/vxfs_super.c	Sat Jul 28 17:08:46 2001
+++ linux/fs/freevxfs/vxfs_super.c	Wed Aug  1 22:41:24 2001
@@ -178,7 +178,8 @@
 	}
 
 	if (rsbp->vs_version < 2 || rsbp->vs_version > 4) {
-		printk(KERN_NOTICE "vxfs: unsupported VxFS version (%d)\n", rsbp->vs_version);
+		printk(KERN_NOTICE "vxfs: unsupported VxFS version (%d)\n",
+		       rsbp->vs_version);
 		goto out;
 	}
 
@@ -221,6 +222,7 @@
 	if (vxfs_read_fshead(sbp)) {
 		printk(KERN_WARNING "vxfs: unable to read fshead\n");
 		return NULL;
+		/* BUG: lots of gets not matched by puts here */
 	}
 
 	sbp->s_op = &vxfs_super_ops;
@@ -229,6 +231,7 @@
 	
 	printk(KERN_WARNING "vxfs: unable to get root dentry.\n");
 out:
+	brelse(bp);
 	kfree(infp);
 	return NULL;
 }

Re: [PATCH] vxfs fix

[Christoph Hellwig]

On Wed, Aug 01, 2001 at 09:03:20PM +0000, Andries.Brouwer@cwi.nl wrote:
> Dear Linus, Alan, Al, Christoph, all,
> 
> If one mounts without specifying a type, mount will try
> all available types. After having tried vxfs the next type
> will cause
> 	set_blocksize: b_count 1 ...
> since vxfs forgets to free a block.
> The patch below adds the missing brelse().
> (In fact there are more resources that are never freed there -
> maybe the maintainer can have a look some time -
> I only added a comment.)

Yes, I already hit that one time but forgot to fix it.
I'll update vxfs to free all resources somewhen later this week.

Thanks for the catch!

> When mount continues to try all types, it may try V7.
> That always succeeds, there is no test for magic or so,
> and after garbage has been mounted as a V7 filesystem,
> the kernel crashes or hangs or fails in other sad ways.
> Have not tried to debug.

Maybe some sanity checks should be added?

I'd like to propose:

	s_nfree <= V7_NICFREE
	s_ninode <= V7_NICINOD
	s_time != 0

Christoph

-- 
Of course it doesn't work. We've performed a software upgrade.

Re: [PATCH] vxfs fix

[Alexander Viro]

On Wed, 1 Aug 2001 Andries.Brouwer@cwi.nl wrote:

> When mount continues to try all types, it may try V7.
> That always succeeds, there is no test for magic or so,
> and after garbage has been mounted as a V7 filesystem,
> the kernel crashes or hangs or fails in other sad ways.
> Have not tried to debug.

That's interesting. IIRC, v7 has no magic in superblock. At all.
It shouldn't crash the box, though - I'll look into that.

Re: [PATCH] vxfs fix

[Linus Torvalds]

On Wed, 1 Aug 2001 Andries.Brouwer@cwi.nl wrote:
>
> When mount continues to try all types, it may try V7.
> That always succeeds, there is no test for magic or so,
> and after garbage has been mounted as a V7 filesystem,
> the kernel crashes or hangs or fails in other sad ways.

Even on filesystems that have bad (limited or non-existent) magic numbers,
the read_super() function should really be able to do a fair amount of
sanity-checking. If nothing else, then things like verifying that the root
inode really is a directory with a proper size, for example.

I don't think V7 has a magic number at all. But checking that the size and
nr-of-inodes fields make sense, together with verifying that the root
inode really is a directory with (size % 512) == 0, and possibly verifying
things like "if the root directory is not large enough to have a
doubly/triply indirect block, then that doubly/triply indirect blocknumber
had better be zero"  would catch 99.9% of everything.

Whether those kind of tests are really worth adding, considering the
rather limited number of people who actually enable the FS, I don't know.

		Linus

Re: [PATCH] vxfs fix

[Alan Cox]

> I don't think V7 has a magic number at all. But checking that the size and
> nr-of-inodes fields make sense, together with verifying that the root
> inode really is a directory with (size % 512) == 0, and possibly verifying
> things like "if the root directory is not large enough to have a
> doubly/triply indirect block, then that doubly/triply indirect blocknumber
> had better be zero"  would catch 99.9% of everything.

Alternatively pass a flag to the mount command saying "this is a guesswork
special" then V7 fs can just return 'not me'

Alan

Re: [PATCH] vxfs fix

[Andries.Brouwer]

	From: Linus Torvalds <torvalds@transmeta.com>

	On Wed, 1 Aug 2001 Andries.Brouwer@cwi.nl wrote:
	>
	> When mount continues to try all types, it may try V7.
	> That always succeeds, there is no test for magic or so,
	> and after garbage has been mounted as a V7 filesystem,
	> the kernel crashes or hangs or fails in other sad ways.

	Even on filesystems that have bad (limited or non-existent)
	magic numbers, 	the read_super() function should really be able
	to do a fair amount of sanity-checking. If nothing else,
	then things like verifying that the root inode really is
	a directory with a proper size, for example.

Added.

	From: Christoph Hellwig

	I'd like to propose:
	s_nfree <= V7_NICFREE, s_ninode <= V7_NICINOD, s_time != 0

Added.

	From: Al Viro

	It shouldn't crash the box, though - I'll look into that.

Good!

	From: Alan

	Alternatively pass a flag to the mount command saying
	"this is a guesswork special" then V7 fs can just return 'not me'

Parse failure.

Below a patch.

Andries


diff -r -u ../linux-2.4.7/linux/fs/sysv/super.c linux/fs/sysv/super.c
--- ../linux-2.4.7/linux/fs/sysv/super.c	Sat Jul 28 17:08:46 2001
+++ linux/fs/sysv/super.c	Thu Aug  2 19:47:58 2001
@@ -197,6 +197,8 @@
 		sb->sv_bytesex = BYTESEX_BE;
 	else
 		return 0;
+
+	/* BUG? no endian conversion on s_time? */
 	if (sbd->s_time < JAN_1_1980) {
 		/* this is likely to happen on SystemV2 FS */
 		if (sbd->s_type > 3 || sbd->s_type < 1)
@@ -294,7 +296,8 @@
 	sb->sv_toobig_block = 10 + bsize_4 * (1 + bsize_4 * (1 + bsize_4));
 	sb->sv_ind_per_block_bits = n_bits-2;
 
-	sb->sv_ninodes = (sb->sv_firstdatazone - sb->sv_firstinodezone) << sb->sv_inodes_per_block_bits;
+	sb->sv_ninodes = (sb->sv_firstdatazone - sb->sv_firstinodezone)
+		<< sb->sv_inodes_per_block_bits;
 
 	sb->s_blocksize = bsize;
 	sb->s_blocksize_bits = n_bits;
@@ -346,13 +349,10 @@
 	sb->sv_block_base = 0;
 
 	for (i = 0; i < sizeof(flavours)/sizeof(flavours[0]) && !size; i++) {
-		struct buffer_head *next_bh;
-		next_bh = bread(dev, flavours[i].block, BLOCK_SIZE);
-		if (!next_bh)
-			continue;
 		brelse(bh);
-		bh = next_bh;
-
+		bh = bread(dev, flavours[i].block, BLOCK_SIZE);
+		if (!bh)
+			continue;
 		size = flavours[i].test(sb, bh);
 	}
 
@@ -411,8 +411,10 @@
 static struct super_block *v7_read_super(struct super_block *sb,void *data,
 				  int silent)
 {
-	struct buffer_head *bh;
+	struct buffer_head *bh, *bh2 = NULL;
 	kdev_t dev = sb->s_dev;
+	struct v7_super_block *v7sb;
+	struct sysv_inode *v7i;
 
 	if (440 != sizeof (struct v7_super_block))
 		panic("V7 FS: bad super-block size");
@@ -422,23 +424,41 @@
 	sb->sv_type = FSTYPE_V7;
 	sb->sv_bytesex = BYTESEX_PDP;
 
-	set_blocksize(dev,512);
+	set_blocksize(dev, 512);
 
 	if ((bh = bread(dev, 1, 512)) == NULL) {
 		if (!silent)
-			printk("VFS: unable to read V7 FS superblock on device "
-			       "%s.\n", bdevname(dev));
+			printk("VFS: unable to read V7 FS superblock on "
+			       "device %s.\n", bdevname(dev));
 		goto failed;
 	}
 
+	/* plausibility check on superblock */
+	v7sb = (struct v7_super_block *) bh->b_data;
+	if (fs16_to_cpu(sb,v7sb->s_nfree) > V7_NICFREE ||
+	    fs16_to_cpu(sb,v7sb->s_ninode) > V7_NICINOD ||
+	    fs32_to_cpu(sb,v7sb->s_time) == 0)
+		goto failed;
+
+	/* plausibility check on root inode: it is a directory,
+	   with a nonzero size that is a multiple of 16 */
+	if ((bh2 = bread(dev, 2, 512)) == NULL)
+		goto failed;
+	v7i = (struct sysv_inode *)(bh2->b_data + 64);
+	if ((fs16_to_cpu(sb,v7i->i_mode) & ~0777) != S_IFDIR ||
+	    (fs32_to_cpu(sb,v7i->i_size) == 0) ||
+	    (fs32_to_cpu(sb,v7i->i_size) & 017) != 0)
+		goto failed;
+	brelse(bh2);
 
 	sb->sv_bh1 = bh;
 	sb->sv_bh2 = bh;
 	if (complete_read_super(sb, silent, 1))
 		return sb;
 
-	brelse(bh);
 failed:
+	brelse(bh2);
+	brelse(bh);
 	return NULL;
 }
 

Re: [PATCH] vxfs fix

[Alan Cox]

> 	From: Alan
> 
> 	Alternatively pass a flag to the mount command saying
> 	"this is a guesswork special" then V7 fs can just return 'not me'
> 
> Parse failure.

Let me try again:

When the read_super method is invoked
AND we are doing a mount without a defined type
	THEN
		Pass the fs a flag from the VFS saying so
	ENDIF

That way the file system can actually say "I cannot reliably check"

Re: [PATCH] vxfs fix

[Andreas Dilger]

Alan writes:
> When the read_super method is invoked
> AND we are doing a mount without a defined type
> 	THEN
> 		Pass the fs a flag from the VFS saying so
> 	ENDIF
> 
> That way the file system can actually say "I cannot reliably check"

Isn't this what the "silent" option to read_super is for?  It may be that
it can only be used at root fs mount time.  Other than that, I don't
_think_ the kernel does autoprobing of filesystem types, so it is a
mount(8) issue to just not randomly try the V7 filesystem type.

Cheers, Andreas
-- 
Andreas Dilger  \ "If a man ate a pound of pasta and a pound of antipasto,
                 \  would they cancel out, leaving him still hungry?"
http://www-mddsp.enel.ucalgary.ca/People/adilger/               -- Dogbert

Re: [PATCH] vxfs fix

[Andries.Brouwer]

> Let me try again

OK - now I see what you mean, but I think you misunderstand.
The system call mount(2) has an obligatory parameter "type".
The kernel never guesses.

[Apart from the situation at boot time, where it knows the
device and guesses the type. This is bad, and I hope it
will go away. Some types are very difficult to distinguish.]

So, the kernel only does what it is told.

But mount(8) has users, and they are a lazy bunch.
Instead of saying "mount -t iso9660 image /mnt -o loop,ro"
they type "mount image /mnt" and hope that mount(8) can
figure out the rest. And mount(8) knows about the magic numbers
of a handful of filesystems, and if one of these is recognized
it will try that type. Otherwise it will just try all remaining
types in /proc/filesystems. It is this guessing that leads to
crashes in case the kernel succeeds in mounting garbage.

Andries