[PATCH] try to avoid false positives on FAT filesystem

[PATCH] try to avoid false positives on FAT filesystem

[Felix Zielcke]

Here's a little patch to check if
bpb.version_specific.fat12_or_fat16.fstype or
bpb.version_specific.fat32.fstype has the string FAT12/FAT16/FAT32
As can be seen on [0] and [1] Dell PCs have a small FAT partition
containing some utilities.
It seems like mkfs.ext2 isn't always clearing the first 512 bytes.
Good for us is that fstype is set to .AT16 on these Dell partitions, so
it can be easily avoided.
Are there any objections?

[0] http://bugs.debian.org/514263
[1] http://bugs.debian.org/505137
-- 
Felix Zielcke

Re: [PATCH] try to avoid false positives on FAT filesystem

[phcoder]

If I understood you correctly you propose to use partition table 
information to determine filesystem. I'm personally against it. First of 
all it isn't portable - it probably wouldn't work on GPT. Second if you 
just do mkfs.* <device> the partition type isn't changed. Linux ignores 
partition type and so if your patch is applied there will be many bug 
reports like "hey, *FS isn't detected". Finally sometimes you 
intentionally change partition type in the technics like partition 
hiding. IMO the correct solution to this problem is to make sure that 
fat and ntfs are probed at last. Should we add a priority field for this?
Thanks
Vladimir 'phcoder' Serbinenko
Felix Zielcke wrote:
> Here's a little patch to check if
> bpb.version_specific.fat12_or_fat16.fstype or
> bpb.version_specific.fat32.fstype has the string FAT12/FAT16/FAT32
> As can be seen on [0] and [1] Dell PCs have a small FAT partition
> containing some utilities.
> It seems like mkfs.ext2 isn't always clearing the first 512 bytes.
> Good for us is that fstype is set to .AT16 on these Dell partitions, so
> it can be easily avoided.
> Are there any objections?
> 
> [0] http://bugs.debian.org/514263
> [1] http://bugs.debian.org/505137

Re: [PATCH] try to avoid false positives on FAT filesystem

[Felix Zielcke]

Am Freitag, den 06.02.2009, 07:07 +0100 schrieb phcoder:
> If I understood you correctly you propose to use partition table 
> information to determine filesystem. I'm personally against it. First of 
> all it isn't portable - it probably wouldn't work on GPT. Second if you 
> just do mkfs.* <device> the partition type isn't changed. Linux ignores 
> partition type and so if your patch is applied there will be many bug 
> reports like "hey, *FS isn't detected". Finally sometimes you 
> intentionally change partition type in the technics like partition 
> hiding. IMO the correct solution to this problem is to make sure that 
> fat and ntfs are probed at last. Should we add a priority field for this?
No I don't want to check the partition type.
FAT partitions normally have the string FAT12/FAT16/FAT32 at the
beginning of 512 bytes of the partition. But these DELL FAT partions
have there the string .AT16 so it would be a solution for the Debian bug
report we got.
But yes the best would be to probe for FAT at last but I don't know how
to change the order, in fs.lst FAT is listed after ext2 but it seems
that FAT is checked before ext2.

-- 
Felix Zielcke

Re: [PATCH] try to avoid false positives on FAT filesystem

[phcoder]

Currently GRUB tries first the filesystem which was loaded at last. If 
you do
rmmod ext2
insmod ext2
then ext2 will be probed first. If we want reliable ordering we need 
priority field.
Thanks
Vladimir 'phcoder' Serbinenko

Felix Zielcke wrote:
> Am Freitag, den 06.02.2009, 07:07 +0100 schrieb phcoder:
>> If I understood you correctly you propose to use partition table 
>> information to determine filesystem. I'm personally against it. First of 
>> all it isn't portable - it probably wouldn't work on GPT. Second if you 
>> just do mkfs.* <device> the partition type isn't changed. Linux ignores 
>> partition type and so if your patch is applied there will be many bug 
>> reports like "hey, *FS isn't detected". Finally sometimes you 
>> intentionally change partition type in the technics like partition 
>> hiding. IMO the correct solution to this problem is to make sure that 
>> fat and ntfs are probed at last. Should we add a priority field for this?
> No I don't want to check the partition type.
> FAT partitions normally have the string FAT12/FAT16/FAT32 at the
> beginning of 512 bytes of the partition. But these DELL FAT partions
> have there the string .AT16 so it would be a solution for the Debian bug
> report we got.
> But yes the best would be to probe for FAT at last but I don't know how
> to change the order, in fs.lst FAT is listed after ext2 but it seems
> that FAT is checked before ext2.
> 

Re: [PATCH] try to avoid false positives on FAT filesystem

[Felix Zielcke]

Am Freitag, den 06.02.2009, 16:46 +0100 schrieb Felix Zielcke:

> But yes the best would be to probe for FAT at last but I don't know how
> to change the order, in fs.lst FAT is listed after ext2 but it seems
> that FAT is checked before ext2.

Ok for grub-probe and probable grub-emu it seems to easy.
The order of the source files decides this.
So if nobody objects I'd like to move fs/fat.c before fs/ext2.c so it
gets checked last.
This would fix the bug for at least the people who get fat.mod instead
of ext2.mod inside of core.img because there /boot filesystem is
screwed.

-- 
Felix Zielcke

Re: [PATCH] try to avoid false positives on FAT filesystem

[Robert Millan]

On Thu, Feb 05, 2009 at 09:39:20PM +0100, Felix Zielcke wrote:
> Here's a little patch to check if
> bpb.version_specific.fat12_or_fat16.fstype or
> bpb.version_specific.fat32.fstype has the string FAT12/FAT16/FAT32
> As can be seen on [0] and [1] Dell PCs have a small FAT partition
> containing some utilities.
> It seems like mkfs.ext2 isn't always clearing the first 512 bytes.
> Good for us is that fstype is set to .AT16 on these Dell partitions, so
> it can be easily avoided.
> Are there any objections?

There's one, you forgot to attach it ;-)

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] try to avoid false positives on FAT filesystem

[Felix Zielcke]

[-- Attachment #1: Type: text/plain, Size: 258 bytes --]

Am Samstag, den 07.02.2009, 23:52 +0100 schrieb Robert Millan:

> There's one, you forgot to attach it ;-)

Actually I didn't forgot it, I was too lazy. I thought such a simple
change doestn't need a patch send to list :)
Well here it is.

-- 
Felix Zielcke

[-- Attachment #2: move_fat_before_ext2.patch --]
[-- Type: text/x-patch, Size: 5143 bytes --]

2009-02-08  Felix Zielcke  <fzielcke@z-51.de>

	* conf/common.rmk (grub_probe_SOURCES): Move fs/ext2.c before fs/fat.c
	to avoid false posivites with FAT.
	(grub_fstest_SOURCES): Likewise.
	* conf/i386-pc.rmk (grub_emu_SOURCES): Likewise.
	* conf/x86_64-efi.rmk (grub_emu_SOURCES): Likewise.
	* conf/powerpc-ieee1275.rmk (grub_emu_SOURCES): Likewise.
	* conf/i386-coreboot.rmk (grub_emu_SOURCES): Likewise.
	* conf/sparc64-ieee1275.rmk (grub_emu_SOURCES): Likewise.
	* conf/i386-ieee1275.rmk (grub_emu_SOURCES): Likewise.

Index: conf/common.rmk
===================================================================
--- conf/common.rmk	(revision 1980)
+++ conf/common.rmk	(working copy)
@@ -14,7 +14,7 @@ grub_probe_SOURCES = util/grub-probe.c	\
 	kern/device.c kern/disk.c kern/err.c kern/misc.c	\
 	kern/parser.c kern/partition.c kern/file.c		\
 	\
-	fs/affs.c fs/cpio.c fs/ext2.c fs/fat.c fs/hfs.c		\
+	fs/affs.c fs/cpio.c fs/fat.c fs/ext2.c fs/hfs.c		\
 	fs/hfsplus.c fs/iso9660.c fs/udf.c fs/jfs.c fs/minix.c	\
 	fs/ntfs.c fs/ntfscomp.c fs/reiserfs.c fs/sfs.c		\
 	fs/ufs.c fs/xfs.c fs/afs.c fs/tar.c			\
@@ -34,7 +34,7 @@ grub_fstest_SOURCES = util/grub-fstest.c
 	disk/host.c disk/loopback.c  normal/arg.c normal/misc.c		\
 	lib/hexdump.c lib/crc.c commands/blocklist.c commands/ls.c 	\
 	\
-	fs/affs.c fs/cpio.c fs/ext2.c fs/fat.c fs/hfs.c			\
+	fs/affs.c fs/cpio.c fs/fat.c fs/ext2.c fs/hfs.c			\
 	fs/hfsplus.c fs/iso9660.c fs/udf.c fs/jfs.c fs/minix.c		\
 	fs/ntfs.c fs/ntfscomp.c fs/reiserfs.c fs/sfs.c			\
 	fs/ufs.c fs/xfs.c fs/afs.c fs/tar.c				\
Index: conf/i386-pc.rmk
===================================================================
--- conf/i386-pc.rmk	(revision 1980)
+++ conf/i386-pc.rmk	(working copy)
@@ -134,7 +134,7 @@ grub_emu_SOURCES = commands/boot.c comma
 	partmap/amiga.c	partmap/apple.c partmap/pc.c partmap/sun.c	\
 	partmap/acorn.c partmap/gpt.c					\
 	\
-	fs/affs.c fs/cpio.c fs/ext2.c fs/fat.c fs/hfs.c			\
+	fs/affs.c fs/cpio.c  fs/fat.c fs/ext2.c fs/hfs.c		\
 	fs/hfsplus.c fs/iso9660.c fs/udf.c fs/jfs.c fs/minix.c		\
 	fs/ntfs.c fs/ntfscomp.c fs/reiserfs.c fs/sfs.c			\
 	fs/ufs.c fs/xfs.c fs/afs.c fs/tar.c				\
Index: conf/x86_64-efi.rmk
===================================================================
--- conf/x86_64-efi.rmk	(revision 1980)
+++ conf/x86_64-efi.rmk	(working copy)
@@ -42,7 +42,7 @@ grub_emu_SOURCES = commands/boot.c comma
 	commands/i386/cpuid.c						\
 	disk/loopback.c							\
 	\
-	fs/affs.c fs/cpio.c fs/ext2.c fs/fat.c fs/hfs.c			\
+	fs/affs.c fs/cpio.c fs/fat.c fs/ext2.c fs/hfs.c			\
 	fs/hfsplus.c fs/iso9660.c fs/udf.c fs/jfs.c fs/minix.c		\
 	fs/ntfs.c fs/ntfscomp.c fs/reiserfs.c fs/sfs.c			\
 	fs/ufs.c fs/xfs.c fs/afs.c					\
Index: conf/powerpc-ieee1275.rmk
===================================================================
--- conf/powerpc-ieee1275.rmk	(revision 1980)
+++ conf/powerpc-ieee1275.rmk	(working copy)
@@ -45,7 +45,7 @@ grub_emu_SOURCES = commands/boot.c comma
 	lib/hexdump.c commands/halt.c commands/reboot.c			\
 	disk/loopback.c							\
 	\
-	fs/affs.c fs/cpio.c fs/ext2.c fs/fat.c fs/hfs.c			\
+	fs/affs.c fs/cpio.c fs/ext2.c fs/fat.c fs/ext2.c fs/hfs.c	\
 	fs/hfsplus.c fs/iso9660.c fs/udf.c fs/jfs.c fs/minix.c		\
 	fs/ntfs.c fs/ntfscomp.c fs/reiserfs.c fs/sfs.c			\
 	fs/ufs.c fs/xfs.c fs/afs.c fs/tar.c				\
Index: conf/i386-coreboot.rmk
===================================================================
--- conf/i386-coreboot.rmk	(revision 1980)
+++ conf/i386-coreboot.rmk	(working copy)
@@ -62,7 +62,7 @@ grub_emu_SOURCES = commands/boot.c comma
 	lib/hexdump.c commands/i386/cpuid.c				\
 	disk/host.c disk/loopback.c					\
 	\
-	fs/affs.c fs/cpio.c fs/ext2.c fs/fat.c fs/hfs.c			\
+	fs/affs.c fs/cpio.c fs/fat.c fs/ext2.c  fs/hfs.c		\
 	fs/hfsplus.c fs/iso9660.c fs/udf.c fs/jfs.c fs/minix.c		\
 	fs/ntfs.c fs/ntfscomp.c fs/reiserfs.c fs/sfs.c			\
 	fs/ufs.c fs/xfs.c fs/afs.c fs/tar.c				\
Index: conf/sparc64-ieee1275.rmk
===================================================================
--- conf/sparc64-ieee1275.rmk	(revision 1980)
+++ conf/sparc64-ieee1275.rmk	(working copy)
@@ -48,7 +48,7 @@ grub_mkimage_SOURCES = util/sparc64/ieee
 #	commands/timeout.c commands/test.c				\
 #	commands/halt.c commands/reboot.c		\
 #	disk/loopback.c							\
-#	fs/affs.c fs/ext2.c fs/fat.c fs/fshelp.c fs/hfs.c fs/iso9660.c  \
+#	fs/affs.c fs/fat.c fs/ext2.c fs/fshelp.c fs/hfs.c fs/iso9660.c  \
 #	fs/jfs.c fs/minix.c fs/sfs.c fs/ufs.c fs/xfs.c                  \
 #	grub_script.tab.c						\
 #	io/gzio.c                                                       \
Index: conf/i386-ieee1275.rmk
===================================================================
--- conf/i386-ieee1275.rmk	(revision 1980)
+++ conf/i386-ieee1275.rmk	(working copy)
@@ -61,7 +61,7 @@ grub_emu_SOURCES = commands/boot.c comma
 	commands/i386/cpuid.c						\
 	disk/host.c disk/loopback.c					\
 	\
-	fs/affs.c fs/cpio.c fs/ext2.c fs/fat.c fs/hfs.c			\
+	fs/affs.c fs/cpio.c fs/fat.c fs/ext2.c fs/hfs.c			\
 	fs/hfsplus.c fs/iso9660.c fs/udf.c fs/jfs.c fs/minix.c		\
 	fs/ntfs.c fs/ntfscomp.c fs/reiserfs.c fs/sfs.c			\
 	fs/ufs.c fs/xfs.c fs/afs.c fs/tar.c				\

Re: [PATCH] try to avoid false positives on FAT filesystem

[Robert Millan]

On Sun, Feb 08, 2009 at 10:03:43AM +0100, Felix Zielcke wrote:
> Am Samstag, den 07.02.2009, 23:52 +0100 schrieb Robert Millan:
> 
> > There's one, you forgot to attach it ;-)
> 
> Actually I didn't forgot it, I was too lazy. I thought such a simple
> change doestn't need a patch send to list :)
> Well here it is.

This looks different than what you described:

> Here's a little patch to check if
> bpb.version_specific.fat12_or_fat16.fstype or
> bpb.version_specific.fat32.fstype has the string FAT12/FAT16/FAT32

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] try to avoid false positives on FAT filesystem

[Felix Zielcke]

Am Sonntag, den 08.02.2009, 19:02 +0100 schrieb Robert Millan:

> 
> This looks different than what you described:
> 
> > Here's a little patch to check if
> > bpb.version_specific.fat12_or_fat16.fstype or
> > bpb.version_specific.fat32.fstype has the string FAT12/FAT16/FAT32

Uhm for this change I did provide a full patch with the code change.

-- 
Felix Zielcke

Re: [PATCH] try to avoid false positives on FAT filesystem

[Robert Millan]

On Sun, Feb 08, 2009 at 08:18:07PM +0100, Felix Zielcke wrote:
> Am Sonntag, den 08.02.2009, 19:02 +0100 schrieb Robert Millan:
> 
> > 
> > This looks different than what you described:
> > 
> > > Here's a little patch to check if
> > > bpb.version_specific.fat12_or_fat16.fstype or
> > > bpb.version_specific.fat32.fstype has the string FAT12/FAT16/FAT32
> 
> Uhm for this change I did provide a full patch with the code change.

I reviewed this thread and couldn't find it.  Did you send it elsewhere?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] try to avoid false positives on FAT filesystem

[Felix Zielcke]

[-- Attachment #1: Type: text/plain, Size: 238 bytes --]

Am Sonntag, den 08.02.2009, 20:56 +0100 schrieb Robert Millan:

> I reviewed this thread and couldn't find it.  Did you send it elsewhere?

In my first message where you quoted the changelog entry.
Well again attached.

-- 
Felix Zielcke

[-- Attachment #2: fat_avoid_fp.diff --]
[-- Type: text/x-patch, Size: 913 bytes --]

2009-02-05  Felix Zielcke  <fzielcke@z-51.de>

	fs/fat.c (grub_fat_mount): Try to avoid false positives by checking
	bpb.version_specific.fat12_or_fat16.fstype and
	bpb.version_specific.fat32.fstype.

Index: fs/fat.c
===================================================================
--- fs/fat.c	(revision 1973)
+++ fs/fat.c	(working copy)
@@ -187,6 +187,11 @@ grub_fat_mount (grub_disk_t disk)
   if (grub_disk_read (disk, 0, 0, sizeof (bpb), (char *) &bpb))
     goto fail;
 
+  if (! grub_strncmp((const char *)bpb.version_specific.fat12_or_fat16.fstype,"FAT12",5)
+      || ! grub_strncmp((const char *)bpb.version_specific.fat12_or_fat16.fstype,"FAT16",5)
+      || ! grub_strncmp((const char *)bpb.version_specific.fat32.fstype,"FAT32",5))
+    goto fail;
+  
   /* Get the sizes of logical sectors and clusters.  */
   data->logical_sector_bits =
     fat_log2 (grub_le_to_cpu16 (bpb.bytes_per_sector));

Re: [PATCH] try to avoid false positives on FAT filesystem

[Robert Millan]

On Sun, Feb 08, 2009 at 08:59:58PM +0100, Felix Zielcke wrote:
> Am Sonntag, den 08.02.2009, 20:56 +0100 schrieb Robert Millan:
> 
> > I reviewed this thread and couldn't find it.  Did you send it elsewhere?
> 
> In my first message where you quoted the changelog entry.
> Well again attached.

Sorry, I must have lost it.

> 2009-02-05  Felix Zielcke  <fzielcke@z-51.de>
> 
> 	fs/fat.c (grub_fat_mount): Try to avoid false positives by checking
> 	bpb.version_specific.fat12_or_fat16.fstype and
> 	bpb.version_specific.fat32.fstype.
> 
> Index: fs/fat.c
> ===================================================================
> --- fs/fat.c	(revision 1973)
> +++ fs/fat.c	(working copy)
> @@ -187,6 +187,11 @@ grub_fat_mount (grub_disk_t disk)
>    if (grub_disk_read (disk, 0, 0, sizeof (bpb), (char *) &bpb))
>      goto fail;
>  
> +  if (! grub_strncmp((const char *)bpb.version_specific.fat12_or_fat16.fstype,"FAT12",5)
> +      || ! grub_strncmp((const char *)bpb.version_specific.fat12_or_fat16.fstype,"FAT16",5)
> +      || ! grub_strncmp((const char *)bpb.version_specific.fat32.fstype,"FAT32",5))
> +    goto fail;
> +  
>    /* Get the sizes of logical sectors and clusters.  */
>    data->logical_sector_bits =
>      fat_log2 (grub_le_to_cpu16 (bpb.bytes_per_sector));

Looks good to me.  But please add spaces between ')' and 'b', and after ','.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] try to avoid false positives on FAT filesystem

[Felix Zielcke]

Am Montag, den 09.02.2009, 15:07 +0100 schrieb Robert Millan:

> Looks good to me.  But please add spaces between ')' and 'b', and after ','.

Ok commited.

-- 
Felix Zielcke

Re: [PATCH] try to avoid false positives on FAT filesystem

[Robert Millan]

On Sun, Feb 08, 2009 at 10:03:43AM +0100, Felix Zielcke wrote:
> 2009-02-08  Felix Zielcke  <fzielcke@z-51.de>
> 
> 	* conf/common.rmk (grub_probe_SOURCES): Move fs/ext2.c before fs/fat.c
> 	to avoid false posivites with FAT.
> 	(grub_fstest_SOURCES): Likewise.
> 	* conf/i386-pc.rmk (grub_emu_SOURCES): Likewise.
> 	* conf/x86_64-efi.rmk (grub_emu_SOURCES): Likewise.
> 	* conf/powerpc-ieee1275.rmk (grub_emu_SOURCES): Likewise.
> 	* conf/i386-coreboot.rmk (grub_emu_SOURCES): Likewise.
> 	* conf/sparc64-ieee1275.rmk (grub_emu_SOURCES): Likewise.
> 	* conf/i386-ieee1275.rmk (grub_emu_SOURCES): Likewise.
> 
> Index: conf/common.rmk
> ===================================================================
> --- conf/common.rmk	(revision 1980)
> +++ conf/common.rmk	(working copy)
> @@ -14,7 +14,7 @@ grub_probe_SOURCES = util/grub-probe.c	\
>  	kern/device.c kern/disk.c kern/err.c kern/misc.c	\
>  	kern/parser.c kern/partition.c kern/file.c		\
>  	\
> -	fs/affs.c fs/cpio.c fs/ext2.c fs/fat.c fs/hfs.c		\
> +	fs/affs.c fs/cpio.c fs/fat.c fs/ext2.c fs/hfs.c		\
>  	fs/hfsplus.c fs/iso9660.c fs/udf.c fs/jfs.c fs/minix.c	\
>  	fs/ntfs.c fs/ntfscomp.c fs/reiserfs.c fs/sfs.c		\
>  	fs/ufs.c fs/xfs.c fs/afs.c fs/tar.c			\

After your other commit, is this one still needed/useful?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] try to avoid false positives on FAT filesystem

[Felix Zielcke]

Am Montag, den 09.02.2009, 15:40 +0100 schrieb Robert Millan:

> 
> After your other commit, is this one still needed/useful?
> 
Well my other commit just fixes the problem probable for these Dell
utiliti FAT partitions because they don't have a valid FAT16 string.
It could be still useful for the people who have a broken mkfs.ext2
which doestn't zero out the first 512 bytes of the partition, but did
had a real valid FAT partition.
And I think it shouldn't hurt to check first for ext2 and then FAT.
-- 
Felix Zielcke

Re: [PATCH] try to avoid false positives on FAT filesystem

[Robert Millan]

On Mon, Feb 09, 2009 at 03:49:46PM +0100, Felix Zielcke wrote:
> Am Montag, den 09.02.2009, 15:40 +0100 schrieb Robert Millan:
> 
> > 
> > After your other commit, is this one still needed/useful?
> > 
> Well my other commit just fixes the problem probable for these Dell
> utiliti FAT partitions because they don't have a valid FAT16 string.
> It could be still useful for the people who have a broken mkfs.ext2
> which doestn't zero out the first 512 bytes of the partition, but did
> had a real valid FAT partition.
> And I think it shouldn't hurt to check first for ext2 and then FAT.

No objection from me.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] try to avoid false positives on FAT filesystem

[Felix Zielcke]

Am Montag, den 09.02.2009, 18:06 +0100 schrieb Robert Millan:

> No objection from me.

Ok commited.

-- 
Felix Zielcke