kern/116608: [panic] [patch] [msdosfs] msdosfs fails to check mount options

[Bruce Evans]

The following reply was made to PR kern/116608; it has been noted by GNATS.

From: Bruce Evans <brde at optusnet.com.au>
To: Eugene Grosbein <eugen at grosbein.pp.ru>
Cc: FreeBSD-gnats-submit at freebsd.org, freebsd-bugs at freebsd.org
Subject: Re: kern/116608: [panic] [patch] [msdosfs] msdosfs fails to check
 mount options
Date: Tue, 25 Sep 2007 04:45:58 +1000 (EST)

 On Mon, 24 Sep 2007, Eugene Grosbein wrote:
 
 >> Description:
 > 	Suppose, there is a line in /etc/fstab:
 >
 > /dev/md0 /mnt/tmp  msdosfs ro,noauto 0 0
 >
 > 	The command 'mount /mnt/tmp' works all right.
 >
 > 	One may try to use 'mount -o rw /mnt/tmp' when wishes
 > 	to mount it read-write initially. It works also, but
 
 I think it should fail because it is missing -u, and -u is only added
 automatically for root (and doesn't seem to be added for you).
 
 > 	any write access to the filesystem returns 'Permission denied'
 > 	from geom layer, so filesystem cannot be unmounted and
 > 	kernel panic is imminent. The reason is that latter command
 > 	translates to 'mount_msdosfs -o ro -o rw /mnt/tmp'
 > 	and vfs_donmount() clears MNT_RDONLY flag for this mount.
 >
 > 	But msdosfs code checks for "ro" option (and does no check for "rw")
 > 	and passes read-only indicator to g_vfs_open().
 
 It may be another bug for the mount to get this far.  For initial mounts,
 MNT_RDONLY is the correct flag to check, while for MNT_UPDATE the setting
 of MNT_RDONLY must remain unchanged until the fs (possibly) toggles it
 near the end of the update, and the request to toggle it must be indicated
 using the "ro" or "rw" option.  However, here we have a mount that is
 neither initial nor an update.  vfs_donmount() thinks it is an initial
 mount so it clears MNT_RDONLY to indicate a "rw" mount.  It might set "rw"
 or clear "ro", but it apparently doesn't.  The msdosfs code is apparently
 picking up the "ro" option from the old mount options (although it looks
 in mnt_optnew)...
 
 >> How-To-Repeat:
 >
 > 	Let's make filesystem to play with (be ready for panic, though)
 >
 > mdconfig -a -t swap -s 1440k
 > newfs_msdosfs -f 1440 /dev/md0
 > mount -o ro -o rw /dev/md0 /mnt/tmp
 >
 > 	(the point of no return)
 >
 > touch /mnt/tmp/file
 >
 > 	Here you'll get EPERM for touch and errors from geom like this:
 >
 > g_vfs_done():md0[WRITE(offset=XXX, length=YYY)]error = 1
 >
 > 	We made it dirty and won't be able to flush buffer,
 > 	so there will be a panic.
 
 I saw similar bogus errors and panics for the bug in rev.1.152 (-current)
 and 1.144.2.5 (RELENG_6).  This bug is for remounting from rw to ro.
 markvoldirty() is called after changing to to, so it creates an unflushable
 buffer.  Unflushable buffers are supposed to be retried endlessly, but
 another bug causes a panic.  Committing of fixes for these bugs are
 pending.
 
 >> Fix:
 >
 > 	One way to fix this is to rely on vfs_donmount's processing
 > 	of mount options for MNT_RDONLY flag instead of using own version,
 > 	because this gives us the behavour we expect: an option that comes
 > 	from command line overrides one coming from fstab.
 >
 > 	Note that this is partial backout (very little one)
 > 	of msdosfs_vfsops.c,1.134
 >
 > --- sys/fs/msdosfs/msdosfs_vfsops.c.orig	2007-09-24 22:16:52.000000000 +0800
 > +++ sys/fs/msdosfs/msdosfs_vfsops.c	2007-09-24 22:49:37.000000000 +0800
 > @@ -417,7 +417,7 @@
 > 	struct g_consumer *cp;
 > 	struct bufobj *bo;
 >
 > -	ronly = !vfs_getopt(mp->mnt_optnew, "ro", NULL, NULL);
 > +	ronly = (mp->mnt_flag & MNT_RDONLY) != 0;
 > 	/* XXX: use VOP_ACCESS to check FS perms */
 > 	DROP_GIANT();
 > 	g_topology_lock();
 
 This fix is already in -current (rev.1.167 = part of fixing root mounts;
 for root mounts, no options are passed; MNT_RDONLY is passed, but when
 it wasn't checked root on msdosfs was always mounted rw and so fsck -p
 on it always failed, causing further problems).  I don't plan to MFC this
 until after 7.0 is released.
 
 ... Multiple mounts (that are not updates) of the same device are supposed
 to work now, if they are all ro.  This case does sort of work (the mounts
 work, but unmount followed by remount panics unless the unmount is in
 stack order; also, mount -v i/o statistics depended on multiple mounts
 not being supported and are now just null).  This seems to create problems
 for your case of a second mount (that is not an update) of the same device
 when the initial mount is ro and the second mount is rw.  I think think
 this should fail with errno EBUSY like it is documented to and used to.  (All
 attempts for multiple mounts are still documented to fail with errno EBUSY.
 The case where the initial mount is rw and the second mount is anything
 still fails, but with a broken errno, EPERM IIRC.  IIRC, the error is
 direct from g_access().)  But for ro to rw without -u, the second mount
 apparently tries to act like an updating mount.  I don't see how this can
 be safe.  I say "apparent" because I haven't checked all the details and
 am arguing based on your patch making a difference.  Your patch is for
 mountmsdosfs(), which is only reached in the non-MNT_UPDATE case.  But
 mountmsdosfs() doesn't do any of the things necessary for an updating
 mount.  I think it just does a complete initial mount, leaving the ro
 mount stacked underneath.  That is dangerous and shouldn't be allowed.
 And how does mountmsdosfs() see the mount flags for the ro mount without
 seeing that the new mount is incompatible with them?
 
 Bruce