Date: Fri, 24 Dec 2010 14:30:12 GMT From: Bruce Evans <brde@optusnet.com.au> To: freebsd-bugs@FreeBSD.org Subject: Re: bin/153426: [patch] fsck_msdosfs only works with sector size 512 Message-ID: <201012241430.oBOEUCNF051150@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR bin/153426; it has been noted by GNATS.
From: Bruce Evans <brde@optusnet.com.au>
To: Keith White <kwhite@uottawa.ca>
Cc: FreeBSD-gnats-submit@FreeBSD.org, freebsd-bugs@FreeBSD.org
Subject: Re: bin/153426: [patch] fsck_msdosfs only works with sector size
512
Date: Sat, 25 Dec 2010 01:23:25 +1100 (EST)
On Fri, 24 Dec 2010, Keith White wrote:
>> Description:
>
> It is possible to create and use an msdos filesystem with 2048-byte
> sectors; but it is not possible to "fsck" it. "fsck_msdosfs" on
> such a fileystem returns:
> "could not read boot block (Invalid argument)"
>
>> Fix:
>
> The following POC patch uses DIOCGSECTORSIZE instead of a fixed value of DOSBOOTBLOCKSIZE.
A much larger patch is needed, since fsck_msdosfs does many other reads
size DOSBOOTBLOCKSIZE or twice that, and to actually fix things it still
tries writing size DOSBOOTBLOCKSIZE for the boot block as well as the
writes corresponding to the other reads.
Also, newfs_msdos used to always put the boot block signature and
fsinfo in wrong places (relative to the end of the physical sector)
when the physical sector size is not 512, so fsck_msdosfs needs to
look in these wrong places as well as the right places, at least for
its signature checks so that it doesn't abort, and consider moving
stuff from the wrong places to the right places (but since msdosfs
just doesn't see stuff in wrong places and doesn't check the primary
signature, it is OK for fsck_msdosfs to ignore the misplaced metadata
and create fresh metadata; this is especially true for fsinfo since
it should be advisory-only).
The unportable DIOCGSECTORSIZE ioctl is not needed and shouldn't be
used, since the boot block contains the sector size that the file
system was created with, and size returned by the ioctl (if any) it
doesn't work unless the media is the same as that on which the file
system was created and everything agrees about it. One case where the
size returned by the ioctl is no good is for fsck on images of file
systems in regular files -- then the ioctl always fail so it doesn't
return any size.
> =============
> --- src/sbin/fsck_msdosfs/boot.c.orig 2010-07-03 06:49:57.000000000 -0400
> +++ src/sbin/fsck_msdosfs/boot.c 2010-12-24 05:49:09.000000000 -0500
> @@ -35,6 +35,7 @@
> #include <string.h>
> #include <stdio.h>
> #include <unistd.h>
> +#include <sys/disk.h>
>
> #include "ext.h"
> #include "fsutil.h"
> @@ -47,11 +48,23 @@
> u_char backup[DOSBOOTBLOCKSIZE];
read()s of size sizeof(backup) and possibly of sizeof(fsinfo) (1K) will
still fail. The failures are fatal, so I think your patch only works
on the unusual non-FAT32 case when these reads aren't attempted, and
when there are also no errors in the boot blocks or fsinfo so no writes
are attempted.
I'm not completely happy with my patch. It preserves some style bugs.
The reblocking is laborious. Perhaps it should be simplified using
utility xread() and xwrite() functions, and make missing signatures
non-fatal. It is very bogus that msdosfs now ignores the primary
signature, while fsck_msdosfs refuses to even look at the file system
when there is no primary signature or the primary signature is where
newfs_msdos used to put it but not where it should be (except with
this patch it accepts it in either place); so fsck_msdosfs can't even
fix common signature problems; these should be handled like corrupted
primary superblocks for ffs (ask but blindly continue if allowed).
% Index: boot.c
% ===================================================================
% RCS file: /home/ncvs/src/sbin/fsck_msdosfs/boot.c,v
% retrieving revision 1.6
% diff -u -2 -r1.6 boot.c
% --- boot.c 31 Jan 2008 13:16:29 -0000 1.6
% +++ boot.c 3 Jul 2010 16:53:33 -0000
% @@ -48,4 +48,6 @@
% #include "fsutil.h"
%
% +#define IOSIZE 65536
% +
% int
% readboot(dosfs, boot)
% @@ -53,18 +55,48 @@
% struct bootblock *boot;
% {
% + u_char ioblock[IOSIZE];
% + u_char iofsinfo[IOSIZE];
% + u_char iobackup[IOSIZE];
% u_char block[DOSBOOTBLOCKSIZE];
% u_char fsinfo[2 * DOSBOOTBLOCKSIZE];
% u_char backup[DOSBOOTBLOCKSIZE];
% + u_char *infop;
% + size_t iosize;
% + u_int secsize;
% int ret = FSOK;
%
% - if (read(dosfs, block, sizeof block) < sizeof block) {
% + /* Search for an i/o size that works. */
% + for (iosize = IOSIZE; iosize >= DOSBOOTBLOCKSIZE; iosize >>= 1) {
% + if (lseek(dosfs, (off_t)0, SEEK_SET) == 0 &&
% + read(dosfs, ioblock, iosize) == (ssize_t)iosize)
% + break;
% + }
% + if (iosize < DOSBOOTBLOCKSIZE) {
% perror("could not read boot block");
% return FSFATAL;
% }
% + memcpy(block, ioblock, sizeof block);
%
% - if (block[510] != 0x55 || block[511] != 0xaa) {
% - pfatal("Invalid signature in boot block: %02x%02x", block[511], block[510]);
% + /*
% + * Preliminary decode to determine where the signature might be.
% + * It is supposed to be at the end of a 512-block, but we used to
% + * put it at the end of a sector. Accept the latter so as to fix
% + * it someday.
% + */
% + secsize = block[11] + (block[12] << 8);
% + if (secsize < sizeof block || secsize > IOSIZE) {
% + perror("Preposterous or unsupported sector size");
% return FSFATAL;
% }
% + if (block[510] != 0x55 || block[511] != 0xaa) {
% + if (ioblock[secsize - 2] != 0x55 ||
% + ioblock[secsize - 1] != 0xaa) {
% + pfatal("Invalid signature in boot block: %02x%02x",
% + block[511], block[510]);
% + return FSFATAL;
% + }
% + pwarn(
% + "Invalid primary signature in boot block -- using secondary\n");
% + }
%
% memset(boot, 0, sizeof *boot);
% @@ -107,11 +139,12 @@
% boot->Backup = block[50] + (block[51] << 8);
%
% + iosize = (secsize >= sizeof fsinfo) ? secsize : sizeof fsinfo;
% if (lseek(dosfs, boot->FSInfo * boot->BytesPerSec, SEEK_SET)
% != boot->FSInfo * boot->BytesPerSec
% - || read(dosfs, fsinfo, sizeof fsinfo)
% - != sizeof fsinfo) {
% + || read(dosfs, iofsinfo, iosize) != (ssize_t)iosize) {
% perror("could not read fsinfo block");
% return FSFATAL;
% }
% + memcpy(fsinfo, iofsinfo, sizeof fsinfo);
% if (memcmp(fsinfo, "RRaA", 4)
% || memcmp(fsinfo + 0x1e4, "rrAa", 4)
% @@ -124,4 +157,22 @@
% || fsinfo[0x3fe] != 0x55
% || fsinfo[0x3ff] != 0xaa) {
% + infop = &iofsinfo[secsize - DOSBOOTBLOCKSIZE];
% + if (memcmp(fsinfo, "RRaA", 4) == 0 &&
% + memcmp(infop + 0x1e4, "rrAa", 4) == 0 &&
% + infop[0x1fc] == 0 &&
% + infop[0x1fd] == 0 &&
% + infop[0x1fe] == 0x55 &&
% + infop[0x1ff] == 0xaa) {
% + pwarn(
% + "Invalid signature in fsinfo block -- using secondary\n");
% + /*
% + * Silently fix up the actual fsinfo data
% + * (just 2 32-bit words) since this data
% + * is advisory and the indentation is
% + * already too painful to ask about this.
% + */
% + memcpy(fsinfo + 0x1e8, infop + 0x1e8, 8);
% + goto over;
% + }
% pwarn("Invalid signature in fsinfo block\n");
% if (ask(0, "Fix")) {
% @@ -134,12 +185,14 @@
% fsinfo[0x3fe] = 0x55;
% fsinfo[0x3ff] = 0xaa;
% + memcpy(iofsinfo, fsinfo, sizeof fsinfo);
% if (lseek(dosfs, boot->FSInfo * boot->BytesPerSec, SEEK_SET)
% != boot->FSInfo * boot->BytesPerSec
% - || write(dosfs, fsinfo, sizeof fsinfo)
% - != sizeof fsinfo) {
% + || write(dosfs, iofsinfo, iosize)
% + != (ssize_t)iosize) {
% perror("Unable to write FSInfo");
% return FSFATAL;
% }
% ret = FSBOOTMOD;
% +over: ;
% } else
% boot->FSInfo = 0;
% @@ -156,8 +209,9 @@
% if (lseek(dosfs, boot->Backup * boot->BytesPerSec, SEEK_SET)
% != boot->Backup * boot->BytesPerSec
% - || read(dosfs, backup, sizeof backup) != sizeof backup) {
% + || read(dosfs, iobackup, secsize) != (ssize_t)secsize) {
% perror("could not read backup bootblock");
% return FSFATAL;
% }
% + memcpy(backup, iobackup, sizeof backup);
% backup[65] = block[65]; /* XXX */
% if (memcmp(block + 11, backup + 11, 79)) {
% @@ -235,12 +299,18 @@
% struct bootblock *boot;
% {
% + u_char iofsinfo[IOSIZE];
% u_char fsinfo[2 * DOSBOOTBLOCKSIZE];
% + size_t iosize;
% + u_int secsize;
%
% + secsize = boot->BytesPerSec;
% + iosize = (secsize >= sizeof fsinfo) ? secsize : sizeof fsinfo;
% if (lseek(dosfs, boot->FSInfo * boot->BytesPerSec, SEEK_SET)
% != boot->FSInfo * boot->BytesPerSec
% - || read(dosfs, fsinfo, sizeof fsinfo) != sizeof fsinfo) {
% + || read(dosfs, iofsinfo, iosize) != (ssize_t)iosize) {
% perror("could not read fsinfo block");
% return FSFATAL;
% }
% + memcpy(fsinfo, iofsinfo, sizeof fsinfo);
% fsinfo[0x1e8] = (u_char)boot->FSFree;
% fsinfo[0x1e9] = (u_char)(boot->FSFree >> 8);
% @@ -251,8 +321,8 @@
% fsinfo[0x1ee] = (u_char)(boot->FSNext >> 16);
% fsinfo[0x1ef] = (u_char)(boot->FSNext >> 24);
% + memcpy(iofsinfo, fsinfo, sizeof fsinfo);
% if (lseek(dosfs, boot->FSInfo * boot->BytesPerSec, SEEK_SET)
% != boot->FSInfo * boot->BytesPerSec
% - || write(dosfs, fsinfo, sizeof fsinfo)
% - != sizeof fsinfo) {
% + || write(dosfs, iofsinfo, iosize) != (ssize_t)iosize) {
% perror("Unable to write FSInfo");
% return FSFATAL;
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201012241430.oBOEUCNF051150>