LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [RFC][PATCH] fs/partitions/msdos: directly check if FAT boot sector
@ 2008-02-28 12:32 Frank Seidel
  2008-02-29  2:16 ` Andreas Dilger
  2008-03-01  1:02 ` Andrew Morton
  0 siblings, 2 replies; 4+ messages in thread
From: Frank Seidel @ 2008-02-28 12:32 UTC (permalink / raw)
  To: Alexander Viro; +Cc: linux-fsdevel, LKML

Hi,

i received a complaint that some FAT formated medias
(e.g. sd memory cards) trigger a "unknown partition table" message even though
there is no partition table and they work correctly, while in general
(when e.g. formated with mkdosfs or even Windows Vista) this message is not
shown.
Currently this seems only to happen when the medias get formatted with
Windows XP (and possibly Win 2000). Then the boot indicator byte contains
garbage (part of text message) and so do the other parts checked by
msdos_paritition which then later triggers this message.

Would the patch below be appropriate to solve this issue/calm those users?
It works ok here for the medias i could test.

Any feedback is very welcome. :-)

Thanks,
Frank
---
From: Frank Seidel <fseidel@suse.de>
Subject: detect fat media without partition table correctly
References: novell bug #364365

Most fat formatted media without partition table contains
zeros in the boot indication and the other tested bytes
and so falls through the checks in msdos_partition, leading
it to return with 1 (all is fine).
But some (e.g. WinXP formatted) fat fomated medias don't
use boot_ind and so the check fails and causes a
"unkown partition table" warning eventhough there is none
and everything would be fine.
This additional check directly verifies if there is a
fat formatted medium without a partition table.

Signed-off-by: Frank Seidel <fseidel@suse.de>
---
 fs/partitions/msdos.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

--- a/fs/partitions/msdos.c
+++ b/fs/partitions/msdos.c
@@ -18,7 +18,7 @@
  *
  *  Re-organised Feb 1998 Russell King
  */
-
+#include <linux/msdos_fs.h>
 
 #include "check.h"
 #include "msdos.h"
@@ -419,6 +419,7 @@ int msdos_partition(struct parsed_partit
 	Sector sect;
 	unsigned char *data;
 	struct partition *p;
+	struct fat_boot_sector *fb;
 	int slot;
 
 	data = read_dev_sector(bdev, 0, &sect);
@@ -441,6 +442,12 @@ int msdos_partition(struct parsed_partit
 	 * partition table. Reject this in case the boot indicator
 	 * is not 0 or 0x80.
 	 */
+	fb = (struct fat_boot_sector *) data;
+	if (fb->reserved && fb->fats && FAT_VALID_MEDIA(fb->media)) {
+		printk("\n");
+		put_dev_sector(sect);
+		return 1;
+	}
 	p = (struct partition *) (data + 0x1be);
 	for (slot = 1; slot <= 4; slot++, p++) {
 		if (p->boot_ind != 0 && p->boot_ind != 0x80) {

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [RFC][PATCH] fs/partitions/msdos: directly check if FAT boot sector
  2008-02-28 12:32 [RFC][PATCH] fs/partitions/msdos: directly check if FAT boot sector Frank Seidel
@ 2008-02-29  2:16 ` Andreas Dilger
  2008-03-01  1:02 ` Andrew Morton
  1 sibling, 0 replies; 4+ messages in thread
From: Andreas Dilger @ 2008-02-29  2:16 UTC (permalink / raw)
  To: Frank Seidel; +Cc: Alexander Viro, linux-fsdevel, LKML

On Feb 28, 2008  13:32 +0100, Frank Seidel wrote:
> i received a complaint that some FAT formated medias
> (e.g. sd memory cards) trigger a "unknown partition table" message even though
> there is no partition table and they work correctly, while in general
> (when e.g. formated with mkdosfs or even Windows Vista) this message is not
> shown.
> Currently this seems only to happen when the medias get formatted with
> Windows XP (and possibly Win 2000). Then the boot indicator byte contains
> garbage (part of text message) and so do the other parts checked by
> msdos_paritition which then later triggers this message.
> 
> Would the patch below be appropriate to solve this issue/calm those users?
> It works ok here for the medias i could test.

There is also a similar complaint from users who format ext3 directly on
a disk/LUN without a partition table, because the partition table offset
negatively impacts the performance of the filesystem (causing unaligned
IO on a RAID device).  Turning off this message is very good.

> ---
> From: Frank Seidel <fseidel@suse.de>
> Subject: detect fat media without partition table correctly
> References: novell bug #364365
> 
> Most fat formatted media without partition table contains
> zeros in the boot indication and the other tested bytes
> and so falls through the checks in msdos_partition, leading
> it to return with 1 (all is fine).
> But some (e.g. WinXP formatted) fat fomated medias don't
> use boot_ind and so the check fails and causes a
> "unkown partition table" warning eventhough there is none
> and everything would be fine.
> This additional check directly verifies if there is a
> fat formatted medium without a partition table.
> 
> Signed-off-by: Frank Seidel <fseidel@suse.de>
> ---
>  fs/partitions/msdos.c |    9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> --- a/fs/partitions/msdos.c
> +++ b/fs/partitions/msdos.c
> @@ -18,7 +18,7 @@
>   *
>   *  Re-organised Feb 1998 Russell King
>   */
> -
> +#include <linux/msdos_fs.h>
>  
>  #include "check.h"
>  #include "msdos.h"
> @@ -419,6 +419,7 @@ int msdos_partition(struct parsed_partit
>  	Sector sect;
>  	unsigned char *data;
>  	struct partition *p;
> +	struct fat_boot_sector *fb;
>  	int slot;
>  
>  	data = read_dev_sector(bdev, 0, &sect);
> @@ -441,6 +442,12 @@ int msdos_partition(struct parsed_partit
>  	 * partition table. Reject this in case the boot indicator
>  	 * is not 0 or 0x80.
>  	 */
> +	fb = (struct fat_boot_sector *) data;
> +	if (fb->reserved && fb->fats && FAT_VALID_MEDIA(fb->media)) {
> +		printk("\n");
> +		put_dev_sector(sect);
> +		return 1;
> +	}
>  	p = (struct partition *) (data + 0x1be);
>  	for (slot = 1; slot <= 4; slot++, p++) {
>  		if (p->boot_ind != 0 && p->boot_ind != 0x80) {
> -
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [RFC][PATCH] fs/partitions/msdos: directly check if FAT boot sector
  2008-02-28 12:32 [RFC][PATCH] fs/partitions/msdos: directly check if FAT boot sector Frank Seidel
  2008-02-29  2:16 ` Andreas Dilger
@ 2008-03-01  1:02 ` Andrew Morton
  2008-03-01  9:38   ` Frank Seidel
  1 sibling, 1 reply; 4+ messages in thread
From: Andrew Morton @ 2008-03-01  1:02 UTC (permalink / raw)
  To: Frank Seidel; +Cc: viro, linux-fsdevel, linux-kernel, OGAWA Hirofumi

On Thu, 28 Feb 2008 13:32:01 +0100
Frank Seidel <fseidel@suse.de> wrote:

> Most fat formatted media without partition table contains
> zeros in the boot indication and the other tested bytes
> and so falls through the checks in msdos_partition, leading
> it to return with 1 (all is fine).
> But some (e.g. WinXP formatted) fat fomated medias don't
> use boot_ind and so the check fails and causes a
> "unkown partition table" warning eventhough there is none
> and everything would be fine.
> This additional check directly verifies if there is a
> fat formatted medium without a partition table.
> 
> Signed-off-by: Frank Seidel <fseidel@suse.de>
> ---
>  fs/partitions/msdos.c |    9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> --- a/fs/partitions/msdos.c
> +++ b/fs/partitions/msdos.c
> @@ -18,7 +18,7 @@
>   *
>   *  Re-organised Feb 1998 Russell King
>   */
> -
> +#include <linux/msdos_fs.h>
>  
>  #include "check.h"
>  #include "msdos.h"
> @@ -419,6 +419,7 @@ int msdos_partition(struct parsed_partit
>  	Sector sect;
>  	unsigned char *data;
>  	struct partition *p;
> +	struct fat_boot_sector *fb;
>  	int slot;
>  
>  	data = read_dev_sector(bdev, 0, &sect);
> @@ -441,6 +442,12 @@ int msdos_partition(struct parsed_partit
>  	 * partition table. Reject this in case the boot indicator
>  	 * is not 0 or 0x80.
>  	 */
> +	fb = (struct fat_boot_sector *) data;
> +	if (fb->reserved && fb->fats && FAT_VALID_MEDIA(fb->media)) {
> +		printk("\n");
> +		put_dev_sector(sect);
> +		return 1;
> +	}
>  	p = (struct partition *) (data + 0x1be);
>  	for (slot = 1; slot <= 4; slot++, p++) {
>  		if (p->boot_ind != 0 && p->boot_ind != 0x80) {

fs/partitions/msdos.c: In function 'msdos_partition':
fs/partitions/msdos.c:446: warning: comparison is always true due to limited range of data type

didn't you get this?

The reason is that FAT_VALID_MEDIA() is bogus:

#define FAT_VALID_MEDIA(x)      ((0xF8 <= (x) && (x) <= 0xFF) || (x) == 0xF0)

It appears that the on-disk field which FAT_VALID_MEDIA() is designed to
test is only 8-bit, so the comparison with 0xff is pointless.  The only
existing caller of FAT_VALID_MEDIA() cheats by copying the value into a
local unsigned int first.

So I'll leave things as they are for now, but I'd ask that someone can
confirm that we should simply remove the 0xff test from FAT_VALID_MEDIA()?


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [RFC][PATCH] fs/partitions/msdos: directly check if FAT boot sector
  2008-03-01  1:02 ` Andrew Morton
@ 2008-03-01  9:38   ` Frank Seidel
  0 siblings, 0 replies; 4+ messages in thread
From: Frank Seidel @ 2008-03-01  9:38 UTC (permalink / raw)
  To: Andrew Morton; +Cc: viro, linux-fsdevel, linux-kernel, OGAWA Hirofumi

Andrew Morton schrieb:
> fs/partitions/msdos.c: In function 'msdos_partition':
> fs/partitions/msdos.c:446: warning: comparison is always true due to limited range of data type
> 
> didn't you get this?

Yes, sorry, but i saw it too late after posting.

> The reason is that FAT_VALID_MEDIA() is bogus:

yes, i stated this in our bug to the complaint
(https://bugzilla.novell.com/show_bug.cgi?id=364365#c15), but should have
also posted it here, sorry.

> #define FAT_VALID_MEDIA(x)      ((0xF8 <= (x) && (x) <= 0xFF) || (x) == 0xF0)
> 
> It appears that the on-disk field which FAT_VALID_MEDIA() is designed to
> test is only 8-bit, so the comparison with 0xff is pointless.  The only
> existing caller of FAT_VALID_MEDIA() cheats by copying the value into a
> local unsigned int first.
> 
> So I'll leave things as they are for now, but I'd ask that someone can
> confirm that we should simply remove the 0xff test from FAT_VALID_MEDIA()?

At least in my opinion it should be removed.

Thanks,
Frank

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2008-03-01  9:39 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-02-28 12:32 [RFC][PATCH] fs/partitions/msdos: directly check if FAT boot sector Frank Seidel
2008-02-29  2:16 ` Andreas Dilger
2008-03-01  1:02 ` Andrew Morton
2008-03-01  9:38   ` Frank Seidel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).