Bug 238579 - msdosfs: Handle duplicate nmount() flags ("shortname"/"shortnames", "longname"/"longnames") correctly
Summary: msdosfs: Handle duplicate nmount() flags ("shortname"/"shortnames", "longname...
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Many People
Assignee: Konstantin Belousov
URL:
Keywords: needs-qa, patch
Depends on:
Blocks:
 
Reported: 2019-06-15 13:55 UTC by Oleg Derevenetz
Modified: 2019-06-25 14:08 UTC (History)
1 user (show)

See Also:
koobs: mfc-stable12?
koobs: mfc-stable11?


Attachments
msdosfs_vfsops.patch (1.20 KB, patch)
2019-06-15 13:55 UTC, Oleg Derevenetz
no flags Details | Diff
Patch with corrected style (1.18 KB, patch)
2019-06-16 09:46 UTC, Oleg Derevenetz
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oleg Derevenetz 2019-06-15 13:55:59 UTC
Created attachment 205074 [details]
msdosfs_vfsops.patch

Currently msdosfs driver doesn't handle duplicate nmount() flags correctly. For example, "shortname" flag doesn't work, while "shortnames" works. The same is for "longname"/"longnames" pair. Proposed patch should fix this.
Comment 1 Konstantin Belousov freebsd_committer 2019-06-15 22:44:18 UTC
The patch looks coorect, it has small style(9) issues.
Comment 2 Oleg Derevenetz 2019-06-16 09:46:44 UTC
Created attachment 205112 [details]
Patch with corrected style

Style (9) issues should be corrected in this version.
Comment 3 Konstantin Belousov freebsd_committer 2019-06-25 14:08:43 UTC
From a bde@ mail:
Since "shortname" and "longname" are undocumented aliases for compatibility
and which never actually worked, the correct fix is to remove them.

This is just another set of bugs in nmount().  The initial conversion
to of msdosfs to nmount() in r138471 on 2004-12-06 misspelled these
options (without an "s"), so broke the documented options.  Then r152595
on 2005-11-28 restored the correct names and kept the misspelled options
for compatibility, but this was apparently not tested since the
misspelled options apparently stopped working.

r152595 doesn't work due to nmount()'s general mishandling of tri-state
logic (default (absent), negative and positive options), and the fix in
the PR is wrong too since it gives the misspelled options precedence...

In -current, vfs_flagopt() on "shortname" sets MSDOSFSMNT_SHORTNAME if
"shortname" is an option and clears the flag otherwise.  This is already
broken for the negative options "noshortname".  Then vfs_flagopt() on
"shortnames" sets MSDOSFSMNT_SHORTNAME if "shortnames" is an option, and
clears the flag otherwise.  So if only "shortname" is an option, then the
flag is always clobbered by checking for "shortnames".  "shortnames" works
accidentally since it is checked last.  Negative options for both are
broken.

In the patch, "shortnames" is not checked for if "shortname" is an option,
so negative options that cancel previous positive options are even further
from working.  E.g., -o shortname,noshortnames is further from working
than -o shortname,noshortname.

nmount() has some support for negative options, but this is implemented very
badly, and is not used for vfs_flagopt() (this does a simple strcmp()).
So even standard negative options like "noro" barely work, and for fs-
specific ones evey file system has the burden of checking for "no"
prefixes.  msdosfs is just missing the checks for "noshortname[s]", so these
negative option just don't work.

Actual testing shows:
- FreeBSD-5.2 is not broken by using nmount() or its spelling errors or
   compatibility cruft for msdosfs, so:
   - -o shortnames works
   - -o shortname fails with the error message "msdosfs: -o shortname: option
     not supported".  Everything is handled in userland and this option is
     just not supported
   - -o shortnames,noshortnames works (the negative option is supported and
     is correctly parsed) in userland and correctly cancels the flag in
     userland.  The userland data for this is 3 bytes long (", 0"):
 	{ "shortnames", 0, MSDOSFSMNT_SHORTNAME, 1 },
 	{ "longnames", 0, MSDOSFSMNT_LONGNAME, 1 },
 	{ "nowin95", 0, MSDOSFSMNT_NOWIN95, 1 },
     The complications for negative logic are in getmntopts(3).  This is
     easy to use, and actually works, unlike nmount() code in the kernel
     which is many times larger.
   - double negative ("nono") prefixes are not supported.

- -current with mount_msdosfs(8) from FreeBSD-~5.2 and FreeBSD-6 shows
   various bugs:
     (I also have mount(8) from FreeBSD-10 which is supposed to be be used
     for -current; it should exec mount_msdosfs from FreeBSD-6 via a script,
     but I now see that my scripts are broken for that use:
     - at boot time, mount(8) is /sbin/mount from ~5.2 and this mostly works
     - later, mount(8) is a script from $HOME/bin.  It uses uname to select
       a mount(8) than works in more cases
     - for -current, the script selects mount(8) from FreeBSD-10
     - similarly for mount_msdosfs(8), except the script selects
       mount_msdosfs(8) from FreeBSD-6
     - mount(8) uses a sanitizied path (not $PATH which includes $HOME/bin),
       so it execs /sbin/mount_msdosfs instead of the script and thus finds
       mount_msdosfs(8) from ~5.2
     - there are similar problems finding mount_nfs(8), but these are more
       noticeable so my scripts already handling them, starting by putting
       the scripts in /sbin which I try not to change.
   Now the bugs:
   - ~5.2 mount_msdosfs uses the parsing utility functions from
     src/sbin/mount, so option strings -o foo,bar,... work.  This is broken
     in at least FreeBSD-6 mount_msdosfs -- a separate -o flag must be used
     for each option.  The man page still says that the syntax is [-o options]
     as described in mount(8).
   - with FreeBSD-6 mount_msdosfs:
     - o shortname fails (silently has no affect, as described above)
     - o shortnames works (as described above)
     - o shortnames,noshortnames fails (is a syntax error, as described above)
     - o shortnames -o noshortnames fails (noshortnames is silently ignored,
       as described above)

"nowin95" is differently broken.  This is a negative option, but is
in msdosfs_opts[] as both "win95" and "nowin95".  The buggy negative
logic apparently allows "noshortnames" to not cause an error although
it is mishandled.  The same should work for "nowin95", but it is
supposed to be a negative option so it should be spelled with "no" in
the options table, but then "win95" might be disallowed.  It is handled
quite differently from "shortnames", using vfs_getopt() instead of
vfs_flagopt() and then laborious setting and clearing of a flag.  Since
the flag has the same negative logic, I don't see why vfs_flagopt()
can't handle it equally badly.  There may be the common problem that we
want the absense of an option to have no effect instead of clearing a
flag.

In practice, "nowin95" is very broken, but not as feared above:
- in ~5.2.  The table entry
 	{ "nowin95", 0, MSDOSFSMNT_NOWIN95, 1 },
   gives a positive option "nowin95", but should give a negative option
   "win95", and its altflag seems to be garbage too.  The correct entry is:
 	{ "win95", 1, MSDOSFSMNT_NOWIN95, 1 /* XXX why not 0? */ },
   This bug obviously makes "win95" an invalid option.  "nowin95" is invalid
   too, apparently because the options parsing removes leading "no" from
   "nowin95" and then looks up "win95" in the table and fails to find it.
   The only option that works is "nonowin95".

   This allows only limited testing of mount(2) in -current: mount ...
   -o shortnames,nonowin95 works correctly -- shortnames kills the default
   of longnames but then nonowin95 restores the default.

- with FreeBSD-6 mount_nfs, -o win95,nowin95 is a syntax error, but
   -o win95 -o nowin95 works correctly and so does -o nowin95 -o win95
   and -o nowin95 -o nonowin95.

nmount()'s mishandling of negative options is now clearer.  For mount(),
getmntinfo() has no need for double negatives like "nonoro", and these
are only allowed by buggy table entries like the one for "nowin95".
Normal entries never start with "no".  Option starting with "nono" are
converted to negative options starting with "no", but these never match
normal entries so are disallowed.  nmount() mostly does the opposite.
E.g., ffs's tables have "noatime" but not "atime", because nmount()
is missing a negative logic flag so has to encode the flag in the name.

The "nowin95" table entry is what allows the double negative "nonowin95"
to work, while "nonoshortnames" is disallowed.  I don't know if the
"win95" table entry is needed to allow "win95" to work.  ffs doesn't
need an "atime" entry to allow "atime" to work -- "atime" is derived
from "noatime".  But standard options like "noatime" are special --
vfs_mount.c has mounds of special cases to handle them.  Individual
file systems might need similar mounds of special cases for nonstandard
options.

Bruce