Re: msdosfs and small sectors
On 09/10/14 23:08, Rhialto wrote: On Wed 10 Sep 2014 at 22:53:27 +0100, David Laight wrote: (and ICL system25 whcih wanted 100 byte sectors). Or IBM's CMS which used to have 800 byte sectors. -Olaf. And there were DEC disks which could be used on both DEC20 (where they were formatted with 9-bit bytes) and VAXen (where they were formatted with 8-bit bytes). If i remember correctly, this mean that while sectors were 512 x 9-bit bytes on a DEC20, on a VAX the same disk would have sectors of 576 x 8 bit bytes. Roger
Re: msdosfs and small sectors
On Wed 10 Sep 2014 at 22:53:27 +0100, David Laight wrote: > (and ICL system25 whcih wanted 100 byte sectors). Or IBM's CMS which used to have 800 byte sectors. -Olaf. -- ___ Olaf 'Rhialto' Seibert -- The Doctor: No, 'eureka' is Greek for \X/ rhialto/at/xs4all.nl-- 'this bath is too hot.' pgpfCrsNfWrkg.pgp Description: PGP signature
Re: msdosfs and small sectors
On Wed, Jul 16, 2014 at 06:26:00PM +, David Holland wrote: > On Wed, Jul 16, 2014 at 03:10:01PM +0200, Maxime Villard wrote: > > I thought about that. I haven't found a clear spec on this, but it is > > implicitly suggested that 512 is the minimal size (from what I've seen > > here and there). And the smallest BytesPerSec allowed for fat devices > > is 512. But still, nothing really clear. > > If you're afraid some real device might turn up with 128-byte sectors > or something, complain if it's less than 64. Or 32. It doesn't really > matter. Real floppies certainly had 128 byte sectors. Some even had 128 byte ones on track 0 but 256 byte ones on the rest of the disk! Is there a check that the sector size is a power of two? That might depend on where it comes from. Real devices with 'unusual' sector sizes do exist (like audio CD), but they won't have a FAT fs on them. (and ICL system25 whcih wanted 100 byte sectors). David -- David Laight: da...@l8s.co.uk
Re: msdosfs and small sectors
Maxime Villard writes:
> Le 17/07/2014 06:01, matthew green a écrit :
+ if (secsize < DEV_BSIZE) {
+#ifdef DIAGNOSTIC
+ printf("Invalid block secsize %d\n", secsize);
+#endif
+ error = EINVAL;
+ goto error_exit;
+ }
if (argp->flags & MSDOSFSMNT_GEMDOSFS) {
if (secsize != GEMDOSFS_BSIZE) {
>>>
>>> That's an abuse of DIAGNOSTIC. It should only be used to control
>>> whether to assert on things that cannot happen. (Bad data on a mount
>>> point is not a software invariant.)
>>
>> agreed -- these sorts of messages should be under DEBUG.
>>
>
> I don't see what's wrong with that; on -current at least if it
> breaks something people will immediately get a clear message, so
> that if we someone says "I get invalid ..." we can quickly revert
> the change.
>
> I think my commit was clear on that:
>
> "Put the printf under DIAGNOSTIC temporarily to see if someone complains."
> ^^^
We are not objecting to the message. The rule for DIAGNOSTIC is that it
only adds asserts. So you are breaking that rule, and doing it for no
good reason. If you want to see if this hits and people see the mesage,
then make the printf unconditional, and put enough context in it so that
someone who isn't looking for this might figure it out.
Longer term, putting it under DEBUG makes sense, as users would be
flooded with messages for every.
So I really did mean to object to even a temporary misuses of
DIAGNOSTIC.
pgpPbBnBhj5c0.pgp
Description: PGP signature
Re: msdosfs and small sectors
Le 17/07/2014 06:01, matthew green a écrit :
>>> + if (secsize < DEV_BSIZE) {
>>> +#ifdef DIAGNOSTIC
>>> + printf("Invalid block secsize %d\n", secsize);
>>> +#endif
>>> + error = EINVAL;
>>> + goto error_exit;
>>> + }
>>>
>>> if (argp->flags & MSDOSFSMNT_GEMDOSFS) {
>>> if (secsize != GEMDOSFS_BSIZE) {
>>
>> That's an abuse of DIAGNOSTIC. It should only be used to control
>> whether to assert on things that cannot happen. (Bad data on a mount
>> point is not a software invariant.)
>
> agreed -- these sorts of messages should be under DEBUG.
>
I don't see what's wrong with that; on -current at least if it
breaks something people will immediately get a clear message, so
that if we someone says "I get invalid ..." we can quickly revert
the change.
I think my commit was clear on that:
"Put the printf under DIAGNOSTIC temporarily to see if someone complains."
^^^
re: msdosfs and small sectors
> > + if (secsize < DEV_BSIZE) {
> > +#ifdef DIAGNOSTIC
> > + printf("Invalid block secsize %d\n", secsize);
> > +#endif
> > + error = EINVAL;
> > + goto error_exit;
> > + }
> >
> > if (argp->flags & MSDOSFSMNT_GEMDOSFS) {
> > if (secsize != GEMDOSFS_BSIZE) {
>
> That's an abuse of DIAGNOSTIC. It should only be used to control
> whether to assert on things that cannot happen. (Bad data on a mount
> point is not a software invariant.)
agreed -- these sorts of messages should be under DEBUG.
.mrg.
Re: msdosfs and small sectors
I've put 512. It's the least magic limit. - Module Name:src Committed By: maxv Date: Wed Jul 16 20:09:00 UTC 2014 Modified Files: src/sys/fs/msdosfs: msdosfs_vfsops.c Log Message: Limit the minimum size of a disk sector to 512 bytes, to prevent memory overflow on extremely low secsize. This normally conforms to the old standard (for which there doesn't seem to be a clear spec). Since 2011, IDEMA's Advanced Format standardizes it to 4k, so this change won't cause any trouble on new devices. Put the printf under DIAGNOSTIC temporarily to see if someone complains. after a quick discussion on tech-kern To generate a diff of this commit: cvs rdiff -u -r1.113 -r1.114 src/sys/fs/msdosfs/msdosfs_vfsops.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. - Thanks, Maxime
Re: msdosfs and small sectors
On Wed, Jul 16, 2014 at 03:10:01PM +0200, Maxime Villard wrote: > I thought about that. I haven't found a clear spec on this, but it is > implicitly suggested that 512 is the minimal size (from what I've seen > here and there). And the smallest BytesPerSec allowed for fat devices > is 512. But still, nothing really clear. If you're afraid some real device might turn up with 128-byte sectors or something, complain if it's less than 64. Or 32. It doesn't really matter. The error message should always print, though. -- David A. Holland dholl...@netbsd.org
Re: msdosfs and small sectors
Maxime Villard writes:
> Le 15/07/2014 17:57, Martin Husemann a écrit :
>>
>> On Tue, Jul 15, 2014 at 03:27:08PM +0200, Maxime Villard wrote:
>>> 'secsize' is retrieved through getdisksize(), via an ioctl on the device.
>>
>> force it to be 512 bytes minimum?
>>
>> Martin
>>
>
> I thought about that. I haven't found a clear spec on this, but it is
> implicitly suggested that 512 is the minimal size (from what I've seen
> here and there). And the smallest BytesPerSec allowed for fat devices
> is 512. But still, nothing really clear.
>
> Index: msdosfs_vfsops.c
> ===
> RCS file: /cvsroot/src/sys/fs/msdosfs/msdosfs_vfsops.c,v
> retrieving revision 1.113
> diff -u -r1.113 msdosfs_vfsops.c
> --- msdosfs_vfsops.c 15 Jul 2014 11:43:54 - 1.113
> +++ msdosfs_vfsops.c 16 Jul 2014 12:58:52 -
> @@ -493,6 +493,13 @@
> psize = 0;
> error = 0;
> }
> + if (secsize < DEV_BSIZE) {
> +#ifdef DIAGNOSTIC
> + printf("Invalid block secsize %d\n", secsize);
> +#endif
> + error = EINVAL;
> + goto error_exit;
> + }
>
> if (argp->flags & MSDOSFSMNT_GEMDOSFS) {
> if (secsize != GEMDOSFS_BSIZE) {
That's an abuse of DIAGNOSTIC. It should only be used to control
whether to assert on things that cannot happen. (Bad data on a mount
point is not a software invariant.)
I don't object to having an error message, but it might be nice to have
the device included, and to say that it's in msdosfs_mount.
I also agree that a value < 512 is almost certainly wrong and it's fine
to error out until there's a valid use case and spec saying smaller is
ok.
pgpJQqsPBMBU2.pgp
Description: PGP signature
Re: msdosfs and small sectors
Le 15/07/2014 17:57, Martin Husemann a écrit :
>
> On Tue, Jul 15, 2014 at 03:27:08PM +0200, Maxime Villard wrote:
>> 'secsize' is retrieved through getdisksize(), via an ioctl on the device.
>
> force it to be 512 bytes minimum?
>
> Martin
>
I thought about that. I haven't found a clear spec on this, but it is
implicitly suggested that 512 is the minimal size (from what I've seen
here and there). And the smallest BytesPerSec allowed for fat devices
is 512. But still, nothing really clear.
Index: msdosfs_vfsops.c
===
RCS file: /cvsroot/src/sys/fs/msdosfs/msdosfs_vfsops.c,v
retrieving revision 1.113
diff -u -r1.113 msdosfs_vfsops.c
--- msdosfs_vfsops.c15 Jul 2014 11:43:54 - 1.113
+++ msdosfs_vfsops.c16 Jul 2014 12:58:52 -
@@ -493,6 +493,13 @@
psize = 0;
error = 0;
}
+ if (secsize < DEV_BSIZE) {
+#ifdef DIAGNOSTIC
+ printf("Invalid block secsize %d\n", secsize);
+#endif
+ error = EINVAL;
+ goto error_exit;
+ }
if (argp->flags & MSDOSFSMNT_GEMDOSFS) {
if (secsize != GEMDOSFS_BSIZE) {
Re: msdosfs and small sectors
On Tue, Jul 15, 2014 at 03:27:08PM +0200, Maxime Villard wrote: > 'secsize' is retrieved through getdisksize(), via an ioctl on the device. force it to be 512 bytes minimum? Martin
msdosfs and small sectors
Hi, some days ago when reading msdosfs_vfsops.c I saw this: if ((error = bread(devvp, 0, secsize, NOCRED, 0, &bp)) != 0) goto error_exit; bsp = (union bootsector *)bp->b_data; b33 = (struct byte_bpb33 *)bsp->bs33.bsBPB; b50 = (struct byte_bpb50 *)bsp->bs50.bsBPB; b710 = (struct byte_bpb710 *)bsp->bs710.bsBPB; 'secsize' is retrieved through getdisksize(), via an ioctl on the device. I have a doubt, isn't there a risk that the kernel overflows memory if secsize is too low? If I plug an USB key with only 2 bytes per sector, only two bytes will be read by this bread(), and 'bp->b_data' will be accessed outside the requested area. Not sure though, does someone have an idea? If I'm right, which limit should we put? Thanks, Maxime
