Re: PATCH: better error return for exFAT filesystem

2020-08-09 Thread Ted Unangst
On 2020-08-09, Jonathan Gray wrote:
> mount_msdos(8) knows about EINVAL and will print "not an MSDOS filesystem"

I think mount_msdos could also inspect the filesytem for the common case of
exfat confusion.

Index: mount_msdos.c
===
RCS file: /home/cvs/src/sbin/mount_msdos/mount_msdos.c,v
retrieving revision 1.34
diff -u -p -r1.34 mount_msdos.c
--- mount_msdos.c   28 Jun 2019 13:32:45 -  1.34
+++ mount_msdos.c   9 Aug 2020 15:10:08 -
@@ -37,6 +37,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -58,6 +59,7 @@ gid_t a_gid(char *);
 uid_t  a_uid(char *);
 mode_t a_mask(char *);
 void   usage(void);
+intcheckexfat(const char *);
 
 int
 main(int argc, char **argv)
@@ -140,6 +142,9 @@ main(int argc, char **argv)
case EINVAL:
errcause =
"not an MSDOS filesystem";
+   if (checkexfat(dev)) {
+   errcause = "exFAT is not supported";
+   }
break;
default:
errcause = strerror(errno);
@@ -204,4 +209,20 @@ usage(void)
fprintf(stderr,
"usage: mount_msdos [-9ls] [-g gid] [-m mask] [-o options] [-u uid] 
special node\n");
exit(1);
+}
+
+int
+checkexfat(const char *dev)
+{
+   char buf[4096];
+   int fd;
+
+   fd = open(dev, O_RDONLY);
+   if (fd != -1) {
+   ssize_t amt = read(fd, buf, sizeof(buf));
+   close(fd);
+   if (amt >= 11 && memcmp(buf + 3, "EXFAT   ", 8) == 0)
+   return 1;
+   }
+   return 0;
 }



Re: PATCH: better error return for exFAT filesystem

2020-08-09 Thread Klemens Nanni
On Sun, Aug 09, 2020 at 07:48:21PM +1000, Jonathan Gray wrote:
> Thinking about this some more the problem is really the choice of errno.
> It used to be EINVAL but was changed to EFTYPE in
> 
> 
> revision 1.7
> date: 1997/06/20 14:04:30;  author: kstailey;  state: Exp;  lines: +7 -7;
> Change errno cause by mounting invalid filesystems from EINVAL to EFTYPE.
> 
> 
> mount_msdos(8) knows about EINVAL and will print "not an MSDOS filesystem"
> 
> On reading mount(2) EOPNOTSUPP would be suitable for when the kernel
> did not have msdos support built in not when the blocks found are not
> msdos.
> 
> So either mount_msdos gains a EFTYPE case or we go back to EINVAL.
Reverting EFTYPE makes sense to me and having mount_msdos say
"not an MSDOS filesystem" on exFAT is an improvement.

OK kn



Re: PATCH: better error return for exFAT filesystem

2020-08-09 Thread Jonathan Gray
On Sat, Aug 08, 2020 at 01:13:20PM +1000, Jonathan Gray wrote:
> On Fri, Aug 07, 2020 at 12:59:00PM -0700, jo...@armadilloaerospace.com wrote:
> > Perform an explicit check for the unsupported exFAT MSDOS filesystem
> > instead of letting it fail mysteriously when it gets cluster sizes
> > of 0 from the normal fields.
> > 
> > This causes mount_msdos to report:
> > mount_msdos: /dev/sd1i on /root/mnt: filesystem not supported by kernel
> > 
> > Instead of the more obscure:
> > mount_msdos: /dev/sd1i on /root/mnt: Inapropriate file type or format
> > 
> > Index: msdosfs_vfsops.c
> > ===
> > RCS file: /cvs/src/sys/msdosfs/msdosfs_vfsops.c,v
> > retrieving revision 1.93
> > diff -u -p -r1.93 msdosfs_vfsops.c
> > --- msdosfs_vfsops.c24 Jan 2020 03:49:34 -  1.93
> > +++ msdosfs_vfsops.c7 Aug 2020 19:52:04 -
> > @@ -298,6 +298,12 @@ msdosfs_mountfs(struct vnode *devvp, str
> > b50 = (struct byte_bpb50 *)bsp->bs50.bsBPB;
> > b710 = (struct byte_bpb710 *)bsp->bs710.bsBPB;
> >  
> > +   /* No support for exFAT filesystems */
> > +   if (!strncmp("EXFAT", bsp->bs33.bsOemName, 5)) {
> > +   error = EOPNOTSUPP;
> > +   goto error_exit;
> > +   }
> > +
> > pmp = malloc(sizeof *pmp, M_MSDOSFSMNT, M_WAITOK | M_ZERO);
> > pmp->pm_mountp = mp;
> 
> The microsoft specification states it is EXFAT followed by three spaces
> 
>   if (!memcmp("EXFAT   ", bsp->bs33.bsOemName, 8)) {
> 
> may be more suitable here.

Thinking about this some more the problem is really the choice of errno.
It used to be EINVAL but was changed to EFTYPE in


revision 1.7
date: 1997/06/20 14:04:30;  author: kstailey;  state: Exp;  lines: +7 -7;
Change errno cause by mounting invalid filesystems from EINVAL to EFTYPE.


mount_msdos(8) knows about EINVAL and will print "not an MSDOS filesystem"

On reading mount(2) EOPNOTSUPP would be suitable for when the kernel
did not have msdos support built in not when the blocks found are not
msdos.

So either mount_msdos gains a EFTYPE case or we go back to EINVAL.

Index: sys/msdosfs/msdosfs_vfsops.c
===
RCS file: /cvs/src/sys/msdosfs/msdosfs_vfsops.c,v
retrieving revision 1.93
diff -u -p -r1.93 msdosfs_vfsops.c
--- sys/msdosfs/msdosfs_vfsops.c24 Jan 2020 03:49:34 -  1.93
+++ sys/msdosfs/msdosfs_vfsops.c9 Aug 2020 09:22:31 -
@@ -321,7 +321,7 @@ msdosfs_mountfs(struct vnode *devvp, str
pmp->pm_BlkPerSec = pmp->pm_BytesPerSec / DEV_BSIZE;
 
if (!pmp->pm_BytesPerSec || !SecPerClust) {
-   error = EFTYPE;
+   error = EINVAL;
goto error_exit;
}
 
@@ -451,7 +451,7 @@ msdosfs_mountfs(struct vnode *devvp, str
 * must be a power of 2
 */
if (pmp->pm_bpcluster ^ (1 << pmp->pm_cnshift)) {
-   error = EFTYPE;
+   error = EINVAL;
goto error_exit;
}
 
Index: sys/ufs/ffs/ffs_vfsops.c
===
RCS file: /cvs/src/sys/ufs/ffs/ffs_vfsops.c,v
retrieving revision 1.185
diff -u -p -r1.185 ffs_vfsops.c
--- sys/ufs/ffs/ffs_vfsops.c24 Jun 2020 22:03:45 -  1.185
+++ sys/ufs/ffs/ffs_vfsops.c9 Aug 2020 09:24:58 -
@@ -754,14 +754,6 @@ ffs_mountfs(struct vnode *devvp, struct 
fs = (struct fs *) bp->b_data;
sbloc = sbtry[i];
 
-#if 0
-   if (fs->fs_magic == FS_UFS2_MAGIC) {
-   printf("ffs_mountfs(): Sorry, no UFS2 support (yet)\n");
-   error = EFTYPE;
-   goto out;
-   }
-#endif
-
/*
 * Do not look for an FFS1 file system at SBLOCK_UFS2. Doing so
 * will find the wrong super-block for file systems with 64k
@@ -813,7 +805,7 @@ ffs_mountfs(struct vnode *devvp, struct 
printf("ffs_mountfs(): obsolete rotational table format, "
"please use fsck_ffs(8) -c 1\n");
 #endif
-   error = EFTYPE;
+   error = EINVAL;
goto out;
}
 



Re: PATCH: better error return for exFAT filesystem

2020-08-07 Thread Jonathan Gray
On Fri, Aug 07, 2020 at 12:59:00PM -0700, jo...@armadilloaerospace.com wrote:
> Perform an explicit check for the unsupported exFAT MSDOS filesystem
> instead of letting it fail mysteriously when it gets cluster sizes
> of 0 from the normal fields.
> 
> This causes mount_msdos to report:
> mount_msdos: /dev/sd1i on /root/mnt: filesystem not supported by kernel
> 
> Instead of the more obscure:
> mount_msdos: /dev/sd1i on /root/mnt: Inapropriate file type or format
> 
> Index: msdosfs_vfsops.c
> ===
> RCS file: /cvs/src/sys/msdosfs/msdosfs_vfsops.c,v
> retrieving revision 1.93
> diff -u -p -r1.93 msdosfs_vfsops.c
> --- msdosfs_vfsops.c  24 Jan 2020 03:49:34 -  1.93
> +++ msdosfs_vfsops.c  7 Aug 2020 19:52:04 -
> @@ -298,6 +298,12 @@ msdosfs_mountfs(struct vnode *devvp, str
>   b50 = (struct byte_bpb50 *)bsp->bs50.bsBPB;
>   b710 = (struct byte_bpb710 *)bsp->bs710.bsBPB;
>  
> + /* No support for exFAT filesystems */
> + if (!strncmp("EXFAT", bsp->bs33.bsOemName, 5)) {
> + error = EOPNOTSUPP;
> + goto error_exit;
> + }
> +
>   pmp = malloc(sizeof *pmp, M_MSDOSFSMNT, M_WAITOK | M_ZERO);
>   pmp->pm_mountp = mp;

The microsoft specification states it is EXFAT followed by three spaces

if (!memcmp("EXFAT   ", bsp->bs33.bsOemName, 8)) {

may be more suitable here.

fsck_msdos(8) likely needs a change to bail early if exFAT is found as
well.



Re: PATCH: better error return for exFAT filesystem

2020-08-07 Thread Klemens Nanni
On Fri, Aug 07, 2020 at 12:59:00PM -0700, jo...@armadilloaerospace.com wrote:
> Perform an explicit check for the unsupported exFAT MSDOS filesystem
> instead of letting it fail mysteriously when it gets cluster sizes
> of 0 from the normal fields.
> 
> This causes mount_msdos to report:
> mount_msdos: /dev/sd1i on /root/mnt: filesystem not supported by kernel
> 
> Instead of the more obscure:
> mount_msdos: /dev/sd1i on /root/mnt: Inapropriate file type or format
Much better.

I think this check for the complete string as per
https://github.com/MicrosoftDocs/win32/blob/docs/desktop-src/FileIO/exfat-specification.md#312-filesystemname-field

The FileSystemName field shall contain the name of the file system on 
the volume.

The valid value for this field is, in ASCII characters, "EXFAT ", which 
includes three trailing white spaces.


/* exFAT specification, 3.1.2 FileSystemName Field */
if (strcmp(bsp->bs33.bsOemName, "EXFAT   ") != 0) {
...
}

> Index: msdosfs_vfsops.c
> ===
> RCS file: /cvs/src/sys/msdosfs/msdosfs_vfsops.c,v
> retrieving revision 1.93
> diff -u -p -r1.93 msdosfs_vfsops.c
> --- msdosfs_vfsops.c  24 Jan 2020 03:49:34 -  1.93
> +++ msdosfs_vfsops.c  7 Aug 2020 19:52:04 -
> @@ -298,6 +298,12 @@ msdosfs_mountfs(struct vnode *devvp, str
>   b50 = (struct byte_bpb50 *)bsp->bs50.bsBPB;
>   b710 = (struct byte_bpb710 *)bsp->bs710.bsBPB;
>  
> + /* No support for exFAT filesystems */
> + if (!strncmp("EXFAT", bsp->bs33.bsOemName, 5)) {
> + error = EOPNOTSUPP;
> + goto error_exit;
> + }
> +
>   pmp = malloc(sizeof *pmp, M_MSDOSFSMNT, M_WAITOK | M_ZERO);
>   pmp->pm_mountp = mp;
>  
> 
>