RFC: Bug in ext2?

RFC: Bug in ext2?

[Carsten Otte]

Hello kernel-list-readers,

I am currently reviewing the second extended fs. I found the following
code in fs/ext2/super.c (taken from kernel 2.4.17):

static int ext2_setup_super (struct super_block * sb,
                     struct ext2_super_block * es,
                     int read_only)
{
     int res = 0;
     if (le32_to_cpu(es->s_rev_level) > EXT2_MAX_SUPP_REV) {
          printk ("EXT2-fs warning: revision level too high, "
               "forcing read-only mode\n");
          res = MS_RDONLY;
     }
     if (read_only)
          return res;
------------------8<-------------------------------*SNIPP*
     ext2_write_super(sb);
------------------8<-------------------------------*SNIPP*
     return res;
}
To me, it looks like if the fs revision in the super block is higher than
the
supported one while read_only is false, the result is set to MS_RDONLY,
but the super block will be written anyway.
I'd expect that MS_RDONLY should be returned at the beginning of the
funcion & the super block of a fs with an unsupported revision should _not_
be written. Am I getting  s.th. wrong?


mit freundlichem Gruß / with kind regards
Carsten Otte

IBM Deutschland Entwicklung GmbH
Linux for eServer development - device driver team
Phone: +49/07031/16-4076
IBM internal phone: *120-4076
--
We are Linux.
Resistance indicates that you're missing the point!

Re: RFC: Bug in ext2?

[Andreas Dilger]

On Jan 16, 2002  14:52 +0100, Carsten Otte wrote:
> I am currently reviewing the second extended fs. I found the following
> code in fs/ext2/super.c (taken from kernel 2.4.17):
> 
> static int ext2_setup_super (struct super_block * sb,
>                      struct ext2_super_block * es,
>                      int read_only)
> {
>      int res = 0;
>      if (le32_to_cpu(es->s_rev_level) > EXT2_MAX_SUPP_REV) {
>           printk ("EXT2-fs warning: revision level too high, "
>                "forcing read-only mode\n");
>           res = MS_RDONLY;
>      }
>      if (read_only)
>           return res;
> ------------------8<-------------------------------*SNIPP*
>      ext2_write_super(sb);
> ------------------8<-------------------------------*SNIPP*
>      return res;
> }

> To me, it looks like if the fs revision in the super block is higher than
> the supported one while read_only is false, the result is set to MS_RDONLY,
> but the super block will be written anyway.
> I'd expect that MS_RDONLY should be returned at the beginning of the
> funcion & the super block of a fs with an unsupported revision should _not_
> be written. Am I getting  s.th. wrong?

You are correct.  This is something that I have fixed a long time in my
tree but have not submitted a patch.  The fix is attached below, but is
extracted from among other changes so may have some offsets or other
minor context issues.  In addition to correctly handling unsupported
revisions, it also returns an error if you try to remount such a fs
as read-write so the user knows there was a problem.

If nobody sees any obvious errors, then please apply.

Cheers, Andreas
======================== ext2-2.4.17-rev.diff ================================
--- linux-2.4.17.orig/fs/ext2/super.c	Thu Dec 13 12:44:33 2001
+++ linux/fs/ext2/super.c	Thu Dec 13 12:22:21 2001
@@ -282,18 +323,17 @@
 	return 1;
 }
 
-static int ext2_setup_super (struct super_block * sb,
-			      struct ext2_super_block * es,
-			      int read_only)
+static int ext2_setup_super(struct super_block *sb, struct ext2_super_block *es,
+			    int read_only)
 {
-	int res = 0;
+	if (read_only)
+		return 0;
 	if (le32_to_cpu(es->s_rev_level) > EXT2_MAX_SUPP_REV) {
 		printk ("EXT2-fs warning: revision level too high, "
 			"forcing read-only mode\n");
-		res = MS_RDONLY;
+		sb->s_flags |= MS_RDONLY;
+		return MS_RDONLY;
 	}
-	if (read_only)
-		return res;
 	if (!(sb->u.ext2_sb.s_mount_state & EXT2_VALID_FS))
 		printk ("EXT2-fs warning: mounting unchecked fs, "
 			"running e2fsck is recommended\n");
@@ -328,7 +368,7 @@
 		ext2_check_inodes_bitmap (sb);
 	}
 #endif
-	return res;
+	return 0;
 }
 
 static int ext2_check_descriptors (struct super_block * sb)
@@ -751,8 +1023,9 @@
 		 * by e2fsck since we originally mounted the partition.)
 		 */
 		sb->u.ext2_sb.s_mount_state = le16_to_cpu(es->s_state);
-		if (!ext2_setup_super (sb, es, 0))
-			sb->s_flags &= ~MS_RDONLY;
+		if (ext2_setup_super(sb, es, 0))
+			return -EROFS;
+		sb->s_flags &= ~MS_RDONLY;
 	}
 	ext2_sync_super(sb, es);
 	return 0;
==============================================================================
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://www-mddsp.enel.ucalgary.ca/People/adilger/