Re: grub-probe detects ext4 wronly as ext2

["Javier Martín" <lordhabbit@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2619 bytes --]

El sáb, 05-07-2008 a las 14:07 +0200, Robert Millan escribió:
> However, adding new strings is expensive, since they tend to take size more
> easily than code.  I would be careful about that.
I checked the space requirements, and seemingly there was a bit of space
available in the .rodata zone, since all those strings only added less
than 20 bytes o_O (at least in i386-pc). Wrapping it up, the pre-post
delta including code and strings is 120 bytes in i386-pc.

> >  grub_ext2_mount (grub_disk_t disk)
> >  {
> >    struct grub_ext2_data *data;
> > +  const char *err_msg = 0;
> 
> Is this "const" right?  You're modifiing its value.
Yeah, err_msg is a "pointer to (const char)", thus the characters are
unmodifiable but the pointer can be reassigned. You can also have "char
* const", which is a "const pointer to char" (I don't see much utility
to it); and finally "const char * const", a "const pointer to (const
char)", which would be the constant, unreassignable string pointer.

AFAIK, since GCC 4.0 string literals are "const char *" by default and
are stored in .rodata, so if the variable was termed as just "char *"
we'd have needed a cast. However, if the compiler considers string
literals as "char *", no cast is needed to make it "const char *".

> >    grub_disk_read (disk, 1 * 2, 0, sizeof (struct grub_ext2_sblock),
> >                    (char *) &data->sblock);
> >    if (grub_errno)
> > -    goto fail;
> > +    EXT2_DRIVER_MOUNT_FAIL("could not read the superblock")
> 
> This overrides the grub_errno and grub_errmsg provided by grub_disk_read and
> replaces them with values that hide the true problem.  If there was a disk
> read error, we really want to know about it from the lower layer.
Well, the old version did just the same (even worse, because the message
was generic). What would be the correct path of action here? I mean, how
can we propagate the error messages?

>  
> >    /* Make sure this is an ext2 filesystem.  */
> >    if (grub_le_to_cpu16 (data->sblock.magic) != EXT2_MAGIC)
> > -    goto fail;
> > +    EXT2_DRIVER_MOUNT_FAIL("not an ext2 filesystem (superblock magic mismatch)")
> 
> No need to ellaborate here;  by definition, the magic number makes the
> difference between a corrupt ext2 and something that is not ext2.  So
> you can just say it's not ext2.
Ok, I'm sending a new version of the thing with that part removed. I'm
trying to think of a ChangeLog entry for this. What about this?
	fs/ext2.c: extN driver will reject filesystems with unimplemented
"incompatible" features enabled in the superblock


[-- Attachment #1.2: ext2_incompat.patch.4 --]
[-- Type: text/x-patch, Size: 4690 bytes --]

Index: fs/ext2.c
===================================================================
--- fs/ext2.c	(revisión: 1691)
+++ fs/ext2.c	(copia de trabajo)
@@ -71,8 +71,53 @@
          ? EXT2_GOOD_OLD_INODE_SIZE \
          : grub_le_to_cpu16 (data->sblock.inode_size))
 
-#define EXT3_FEATURE_COMPAT_HAS_JOURNAL	0x0004
+/* Superblock filesystem feature flags (RW compatible)
+ * A filesystem with any of these enabled can be read and written by a driver
+ * that does not understand them without causing metadata/data corruption */
+#define EXT2_FEATURE_COMPAT_DIR_PREALLOC	0x0001
+#define EXT2_FEATURE_COMPAT_IMAGIC_INODES	0x0002
+#define EXT3_FEATURE_COMPAT_HAS_JOURNAL		0x0004
+#define EXT2_FEATURE_COMPAT_EXT_ATTR		0x0008
+#define EXT2_FEATURE_COMPAT_RESIZE_INODE	0x0010
+#define EXT2_FEATURE_COMPAT_DIR_INDEX		0x0020
+/* Superblock filesystem feature flags (RO compatible)
+ * A filesystem with any of these enabled can be safely read by a driver that
+ * does not understand them, but should not be written to, usually because
+ * additional metadata is required */
+#define EXT2_FEATURE_RO_COMPAT_SPARSE_SUPER	0x0001
+#define EXT2_FEATURE_RO_COMPAT_LARGE_FILE	0x0002
+#define EXT2_FEATURE_RO_COMPAT_BTREE_DIR	0x0004
+#define EXT4_FEATURE_RO_COMPAT_GDT_CSUM		0x0010
+#define EXT4_FEATURE_RO_COMPAT_DIR_NLINK	0x0020
+#define EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE	0x0040
+/* Superblock filesystem feature flags (back-incompatible)
+ * A filesystem with any of these enabled should not be attempted to be read
+ * by a driver that does not understand them, since they usually indicate
+ * metadata format changes that might confuse the reader. */
+#define EXT2_FEATURE_INCOMPAT_COMPRESSION	0x0001
+#define EXT2_FEATURE_INCOMPAT_FILETYPE		0x0002
+#define EXT3_FEATURE_INCOMPAT_RECOVER		0x0004 /* Needs recovery */
+#define EXT3_FEATURE_INCOMPAT_JOURNAL_DEV	0x0008 /* Volume is journal device */
+#define EXT2_FEATURE_INCOMPAT_META_BG		0x0010
+#define EXT4_FEATURE_INCOMPAT_EXTENTS		0x0040 /* Extents used */
+#define EXT4_FEATURE_INCOMPAT_64BIT		0x0080
+#define EXT4_FEATURE_INCOMPAT_FLEX_BG		0x0200
 
+/* The set of back-incompatible features this driver DOES support. Add (OR)
+ * flags here as the related features are implemented into the driver */
+#define EXT2_DRIVER_SUPPORTED_INCOMPAT ( EXT2_FEATURE_INCOMPAT_FILETYPE \
+                                       | EXT4_FEATURE_INCOMPAT_EXTENTS )
+/* List of rationales for the ignored "incompatible" features:
+ * needs_recovery: Not really back-incompatible - was added as such to forbid
+ *                 ext2 drivers from mounting an ext3 volume with a dirty
+ *                 journal because they will ignore the journal, but the next
+ *                 ext3 driver to mount the volume will find the journal and
+ *                 replay it, potentially corrupting the metadata written by
+ *                 the ext2 drivers
+ */
+#define EXT2_DRIVER_IGNORED_INCOMPAT ( EXT3_FEATURE_INCOMPAT_RECOVER )
+
+
 #define EXT3_JOURNAL_MAGIC_NUMBER	0xc03b3998U
 
 #define EXT3_JOURNAL_DESCRIPTOR_BLOCK	1
@@ -375,10 +420,12 @@
   return 0;
 }
 
+#define EXT2_DRIVER_MOUNT_FAIL(message) { err_msg = (message); goto fail; }
 static struct grub_ext2_data *
 grub_ext2_mount (grub_disk_t disk)
 {
   struct grub_ext2_data *data;
+  const char *err_msg = 0;
 
   data = grub_malloc (sizeof (struct grub_ext2_data));
   if (!data)
@@ -388,12 +435,18 @@
   grub_disk_read (disk, 1 * 2, 0, sizeof (struct grub_ext2_sblock),
                   (char *) &data->sblock);
   if (grub_errno)
-    goto fail;
+    EXT2_DRIVER_MOUNT_FAIL("could not read the superblock")
 
   /* Make sure this is an ext2 filesystem.  */
   if (grub_le_to_cpu16 (data->sblock.magic) != EXT2_MAGIC)
-    goto fail;
+    EXT2_DRIVER_MOUNT_FAIL("not an ext2 filesystem")
   
+  /* Check the FS doesn't have feature bits enabled that we don't support. */
+  if (grub_le_to_cpu32 (data->sblock.feature_incompat)
+        & ~(EXT2_DRIVER_SUPPORTED_INCOMPAT | EXT2_DRIVER_IGNORED_INCOMPAT))
+    EXT2_DRIVER_MOUNT_FAIL("filesystem has unsupported incompatible features")
+    
+  
   data->disk = disk;
 
   data->diropen.data = data;
@@ -404,12 +457,14 @@
 
   grub_ext2_read_inode (data, 2, data->inode);
   if (grub_errno)
-    goto fail;
+    EXT2_DRIVER_MOUNT_FAIL("could not read the root directory inode")
   
   return data;
 
  fail:
-  grub_error (GRUB_ERR_BAD_FS, "not an ext2 filesystem");
+  if (!err_msg)
+    err_msg = "DEBUG: mount failed but no error message supplied!";
+  grub_error (GRUB_ERR_BAD_FS, err_msg);
   grub_free (data);
   return 0;
 }

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]