On Fri, 1 Feb 2008, Garrett D'Amore wrote:
Okay, thanks. If you send diffs, I can act as code reviewer, and I can also
validate them, and if necessary, integrate them. (Although we may need to
reverse reviewer and integrator roles in that case.) I was actually planning
on taking a close look at maybe working on this bug myself anyway.
Anyway, just let me know what help, if any, you would like from me.
-- Garrett
Hi Garrett,
have attached a diff for the following ones:
6587983 PCFS pc_validcl() check is incorrect / too restrictive after fix for
5047630
6613462 Write protected zip drive does not get mounted in public zone in TX10x86
6620847 pcfs mount fails on SNV_b75
The first one is likely not 100% complete (adjustments to pc_alloc.c are
advisable there as well, but need to look at the details again - this is
prone to have one-off errors ...) and would need cornercase testing, but
then it's pretty easy to separate that one out if you like.
I don't have a cr.opensolaris.org account yet (had cr.grommit.com but
missed the switchover), so I can't put the webrev in public right now;
Sun-internal, it's at /net/tb3.uk/tank/u/frankho/pcfs-misc/ see there.
I'll be back in April,
Best regards,
FrankH.
Frank Hofmann wrote:
Ok, see this thing now.
Disks can be opened as read/write even if you're requesting a readonly
mount. It seems the flash-based media that are lockable actually refuse
the writeable open hence it fails.
Also, the order of things in pcfs_mount is wrong. It's not so much that
device_identify() is done before parse_mntopts() but rather that the
VOP_OPEN() call is done before then. Quite a few things need to be moved
around:
- the instance structure is needed far earlier - before device_identify
to get everything correct.
- the mount option parsing needs to be done first
- have device_identify (which checks for VFS_RDONLY as well) done after
- have the VOP_OPEN() stuff done after
- make sure that the vfs_bsize assignment is only made after
pc_getfattype() was called - pcfs_clsize isn't initialized before.
Sometimes, it helps if people actually _look_ at the code - thanks !
I'm on holiday starting mid next week; I should be able to send you diffs
out for that fix, but cannot possibly integrate anything by then.
FrankH.
On Fri, 1 Feb 2008, Frank Hofmann wrote:
>
>
> On Fri, 1 Feb 2008, Artem Kachitchkine wrote:
>
> >
> > > [EMAIL PROTECTED]> pfexec truss -f -t mount mount -F pcfs -o ro
> > > /dev/dsk/c2t0d0p0:1 /mnt
> > > 3708: mount("/dev/dsk/c2t0d0p0:1", "/mnt", MS_OPTIONSTR, "pcfs",
> > > 0x00000000, 0, 0x08063240, 1024) Err#30 EROFS
> > > mount: Read-only file system
> >
> > As another data point, submitter of 6642367 reports the same issue
> > with
> > USB-connected SD media on snv_79 and claims that it used to work on
> > snv_64a.
>
> It's a bug. The VOP_OPEN() of the device isn't done with the proper
> flags.
>
> Can't tell you out of my head why the changes to PCFS done inbetween
> those
> two builds caused it. Still, a bug.
>
> Thx,
> FrankH.
>
> >
> > -Artem
_______________________________________________
> > opensolaris-code mailing list
> > [email protected]
> > http://mail.opensolaris.org/mailman/listinfo/opensolaris-code
> >
_______________________________________________
> opensolaris-code mailing list
> [email protected]
> http://mail.opensolaris.org/mailman/listinfo/opensolaris-code
>
------------------------------------------------------------------------------
No good can come from selling your freedom, not for all the gold in the world,
for the value of this heavenly gift far exceeds that of any fortune on earth.
------------------------------------------------------------------------------
--- old/usr/src/uts/common/sys/fs/pc_fs.h Mon Feb 4 14:19:56 2008
+++ new/usr/src/uts/common/sys/fs/pc_fs.h Mon Feb 4 14:19:56 2008
@@ -19,7 +19,7 @@
* CDDL HEADER END
*/
/*
- * Copyright 2007 Sun Microsystems, Inc. All rights reserved.
+ * Copyright 2008 Sun Microsystems, Inc. All rights reserved.
* Use is subject to license terms.
*/
@@ -26,7 +26,7 @@
#ifndef _SYS_FS_PC_FS_H
#define _SYS_FS_PC_FS_H
-#pragma ident "@(#)pc_fs.h 1.38 07/09/21 SMI"
+#pragma ident "@(#)pc_fs.h 1.39 08/02/04 SMI"
#include <sys/thread.h>
#include <sys/ksynch.h>
@@ -275,6 +275,7 @@
*/
#define VALID_EXTFLAGS(flags) (((flags) & 0x8f) == (flags))
+
/*
* Validation results
*/
@@ -419,8 +420,8 @@
int pcfs_fat_changemapsize; /* size of FAT changemap */
time_t pcfs_fattime; /* time FAT becomes invalid */
time_t pcfs_verifytime; /* time to reverify disk */
- kmutex_t pcfs_lock; /* per filesystem lock */
- kthread_id_t pcfs_owner; /* id of thread locking pcfs */
+ kmutex_t pcfs_lock; /* per filesystem lock */
+ kthread_id_t pcfs_owner; /* id of thread locking pcfs */
int pcfs_count; /* # of pcfs locks for pcfs_owner */
struct fat_fsinfo pcfs_fsinfo; /* in-core fsinfo */
struct pcfs *pcfs_nxt; /* linked list of all mounts */
@@ -566,10 +567,16 @@
/*
* out-of-range check for cluster numbers.
+ * There is a difference between a 'FAT cluster number' and a 'FAT[] index',
+ * and that is an offset of two; the first data cluster is FAT[2], and if
+ * the filesystem has N clusters in total, the last valid one will be
+ * FAT[N + 1]. I.e. cluster numbers range s [2 .. ncluster + 1] inclusively,
+ * and the FAT table itself must be large enough to accommodate the two
+ * leading 'special indices' FAT[0] / FAT[1] as well.
*/
-#define pc_validcl(PCFS, CL) /* check that cluster no is
legit */ \
- ((int)(CL) >= PCF_FIRSTCLUSTER && \
- (int)(CL) <= (PCFS)->pcfs_ncluster)
+#define pc_validcl(PCFS, CL)
\
+ ((int)(CL) >= PCF_FIRSTCLUSTER && \
+ (int)(CL) < (PCFS)->pcfs_ncluster + PCF_FIRSTCLUSTER)
/*
* external routines.
--- old/usr/src/uts/common/fs/pcfs/pc_vfsops.c Mon Feb 4 14:19:57 2008
+++ new/usr/src/uts/common/fs/pcfs/pc_vfsops.c Mon Feb 4 14:19:57 2008
@@ -19,11 +19,11 @@
* CDDL HEADER END
*/
/*
- * Copyright 2007 Sun Microsystems, Inc. All rights reserved.
+ * Copyright 2008 Sun Microsystems, Inc. All rights reserved.
* Use is subject to license terms.
*/
-#pragma ident "@(#)pc_vfsops.c 1.104 07/10/25 SMI"
+#pragma ident "@(#)pc_vfsops.c 1.105 08/02/04 SMI"
#include <sys/param.h>
#include <sys/systm.h>
@@ -526,6 +526,7 @@
* it's a workaround for devices (particularly: lofi image files)
* that don't support the DKIOCGMEDIAINFO ioctl for autodetection.
*/
+/* ARGSUSED1 */
static void
pcfs_parse_mntopts(struct pcfs *fsp, struct mounta *uap)
{
@@ -537,11 +538,6 @@
ASSERT(fsp->pcfs_secondswest == 0);
ASSERT(fsp->pcfs_secsize == 0);
- if (uap->flags & MS_RDONLY) {
- vfsp->vfs_flag |= VFS_RDONLY;
- vfs_setmntopt(vfsp, MNTOPT_RO, NULL, 0);
- }
-
if (vfs_optionisset(vfsp, MNTOPT_PCFS_HIDDEN, NULL))
fsp->pcfs_flags |= PCFS_HIDDEN;
if (vfs_optionisset(vfsp, MNTOPT_PCFS_FOLDCASE, NULL))
@@ -651,6 +647,18 @@
"Ignoring mount(2) 'dataptr' argument.");
/*
+ * This is needed early, to make sure the access / open calls
+ * are done using the correct mode. Processing this mount option
+ * only when calling pcfs_parse_mntopts() would lead us to attempt
+ * a read/write access to a possibly writeprotected device, and
+ * a readonly mount attempt might fail because of that.
+ */
+ if (uap->flags & MS_RDONLY) {
+ vfsp->vfs_flag |= VFS_RDONLY;
+ vfs_setmntopt(vfsp, MNTOPT_RO, NULL, 0);
+ }
+
+ /*
* For most filesystems, this is just a lookupname() on the
* mount pathname string. PCFS historically has to do its own
* partition table parsing because not all Solaris architectures
@@ -707,12 +715,6 @@
fsp->pcfs_devvp = devvp;
fsp->pcfs_ldrive = dos_ldrive;
mutex_init(&fsp->pcfs_lock, NULL, MUTEX_DEFAULT, NULL);
- vfsp->vfs_data = fsp;
- vfsp->vfs_dev = pseudodev;
- vfsp->vfs_fstype = pcfstype;
- vfs_make_fsid(&vfsp->vfs_fsid, pseudodev, pcfstype);
- vfsp->vfs_bcount = 0;
- vfsp->vfs_bsize = fsp->pcfs_clsize;
pcfs_parse_mntopts(fsp, uap);
@@ -731,6 +733,17 @@
goto errout;
/*
+ * Now that the BPB has been parsed, this structural information
+ * is available and known to be valid. Initialize the VFS.
+ */
+ vfsp->vfs_data = fsp;
+ vfsp->vfs_dev = pseudodev;
+ vfsp->vfs_fstype = pcfstype;
+ vfs_make_fsid(&vfsp->vfs_fsid, pseudodev, pcfstype);
+ vfsp->vfs_bcount = 0;
+ vfsp->vfs_bsize = fsp->pcfs_clsize;
+
+ /*
* Validate that we can access the FAT and that it is, to the
* degree we can verify here, self-consistent.
*/
@@ -1854,8 +1867,38 @@
* This looks far more complicated that it needs to be for pure structural
* validation. The reason for this is that parseBPB() is also used for
* debugging purposes (mdb dcmd) and we therefore want a bitmap of which
- * BPB fields have 'known good' values, even if we do not reject the BPB
- * when attempting to mount the filesystem.
+ * BPB fields (do not) have 'known good' values, even if we (do not) reject
+ * the BPB when attempting to mount the filesystem.
+ *
+ * Real-world usage of FAT shows there are a lot of corner-case situations
+ * and, following the specification strictly, invalid filesystems out there.
+ * Known are situations such as:
+ * - FAT12/FAT16 filesystems with garbage in either totsec16/32
+ * instead of the zero in one of the fields mandated by the spec
+ * - filesystems that claim to be larger than the partition they're in
+ * - filesystems without valid media descriptor
+ * - FAT32 filesystems with RootEntCnt != 0
+ * - FAT32 filesystems with less than 65526 clusters
+ * - FAT32 filesystems without valid FSI sector
+ * - FAT32 filesystems with FAT size in fatsec16 instead of fatsec32
+ *
+ * Such filesystems are accessible by PCFS - if it'd know to start with that
+ * the filesystem should be treated as a specific FAT type. Before S10, it
+ * relied on the PC/fdisk partition type for the purpose and almost completely
+ * ignored the BPB; now it ignores the partition type for anything else but
+ * logical drive enumeration, which can result in rejection of (invalid)
+ * FAT32 - if the partition ID says FAT32, but the filesystem, for example
+ * has less than 65526 clusters.
+ *
+ * Without a "force this fs as FAT{12,16,32}" tunable or mount option, it's
+ * not possible to allow all such mostly-compliant filesystems in unless one
+ * accepts false positives (definitely invalid filesystems that cause problems
+ * later). This at least allows to pinpoint why the mount failed.
+ *
+ * Due to the use of FAT on removeable media, all relaxations of the rules
+ * here need to be carefully evaluated wrt. to potential effects on PCFS
+ * resilience. A faulty/"mis-crafted" filesystem may not cause a panic, so
+ * beware.
*/
static int
parseBPB(struct pcfs *fsp, uchar_t *bpb, int *valid)
@@ -1924,6 +1967,12 @@
if (fsp->pcfs_mediasize == 0) {
mediasize = (len_t)totsec * (len_t)secsize;
+ /*
+ * This is not an error because not all devices support the
+ * dkio(7i) mediasize queries, and/or not all devices are
+ * partitioned. If we have not been able to figure out the
+ * size of the underlaying medium, we have to trust the BPB.
+ */
PC_DPRINTF4(3, "!pcfs: parseBPB: mediasize autodetect failed "
"on device (%x.%x):%d, trusting BPB totsec (%lld Bytes)\n",
getmajor(fsp->pcfs_xdev), getminor(fsp->pcfs_xdev),
@@ -1989,17 +2038,20 @@
size_t, rdirsec, blkcnt_t, datasec);
/*
- * UINT32_MAX is an underflow check - we calculate in "blkcnt_t" which
- * is 64bit in order to be able to catch "impossible" sector counts.
- * A sector count in FAT must fit 32bit unsigned int.
+ * 'totsec' is taken directly from the BPB and guaranteed to fit
+ * into a 32bit unsigned integer. The calculation of 'datasec',
+ * on the other hand, could underflow for incorrect values in
+ * rdirsec/reserved/fatsec. Check for that.
+ * We also check that the BPB conforms to the FAT specification's
+ * requirement that either of the 16/32bit total sector counts
+ * must be zero.
*/
if (totsec != 0 &&
(totsec16 == totsec32 || totsec16 == 0 || totsec32 == 0) &&
- (len_t)totsec * (len_t)secsize <= mediasize &&
datasec < totsec && datasec <= UINT32_MAX)
validflags |= BPB_TOTSEC_OK;
- if (mediasize >= (len_t)datasec * (len_t)secsize)
+ if ((len_t)totsec * (len_t)secsize <= mediasize)
validflags |= BPB_MEDIASZ_OK;
if (VALID_SECSIZE(secsize))
@@ -2108,7 +2160,8 @@
validflags |= BPB_TOTSEC32_OK;
if (bpb_get_FatSz16(bpb) == fatsec)
validflags |= BPB_FATSZ16_OK;
- if (fatsec * secsize >= ncl * 3 / 2)
+ if (fatsec * secsize >=
+ (ncl + PCF_FIRSTCLUSTER) * 3 / 2)
validflags |= BPB_FATSZ_OK;
if (ncl < 4085)
validflags |= BPB_NCLUSTERS_OK;
@@ -2132,7 +2185,7 @@
validflags |= BPB_TOTSEC32_OK;
if (bpb_get_FatSz16(bpb) == fatsec)
validflags |= BPB_FATSZ16_OK;
- if (fatsec * secsize >= ncl * 2)
+ if (fatsec * secsize >= (ncl + PCF_FIRSTCLUSTER) * 2)
validflags |= BPB_FATSZ_OK;
if (ncl >= 4085 && ncl < 65525)
validflags |= BPB_NCLUSTERS_OK;
@@ -2156,7 +2209,7 @@
validflags |= BPB_FATSZ16_OK;
if (bpb_get_FatSz32(bpb) == fatsec)
validflags |= BPB_FATSZ32_OK;
- if (fatsec * secsize >= ncl * 4)
+ if (fatsec * secsize >= (ncl + PCF_FIRSTCLUSTER) * 4)
validflags |= BPB_FATSZ_OK;
if (ncl >= 65525 && ncl < PCF_LASTCLUSTER32)
validflags |= BPB_NCLUSTERS_OK;
_______________________________________________
opensolaris-code mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/opensolaris-code
