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 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(). 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 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.
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
Bruce Evans 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). It's not about second mount, it's about initial mount - see How-To-Repeat section. > >> 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 > >> 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. I do not expect that root mount of msdosfs would work for RELENG_6, but why not MFC this small fix for the PR? > ... 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? I'm not talking about second mount. Eugene
On Mon, 24 Sep 2007, Bruce Evans wrote: > 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. While the patch I've sent in the PR is correct, it discovers another panic if there is mountd process running at the moment of mount. Or if it's run later. Here is a scenario: 1. mount_msdosfs(8) calls nmount() with options "ro" and "rw" (in this order). 2. vfs_donmount() notes "rw", clears MNT_RDONLY flag and adds "noro" to the option list. 3. So, msdosfs_mount() is called without MNT_UPDATE flag, without MNT_RDONLY flag and with options "ro", "rw" and "noro". It ignores "noro", and with mentioned patch it ignores "ro" also and does read-write mount. 4. mountd(8) calls nmount() with exactly same options but adds MNT_UPDATE flag 5. So, msdosfs_mount() is called second time, now with MNT_UPDATE, ignores options "noro" and "rw", notes option "ro", tries to remount filesystem from read-write to read-only and hits another unfixed bug in the kernel you mentioned above and kernel panices a couple of second later. Well, I think that vfs_donmount() has to be fixed to remove "ro" when it adds "noro", hasn't it? Eugene Grosbein
Hi! Trying to minimize effect of change, I've settled with the following patch: just ignore "ro" options if there is also "noro" option, presumably added within vfs_donmount(). Now it works as expected and does not trigger off another bugs :-) --- sys/fs/msdosfs/msdosfs_vfsops.c.orig 2007-09-24 22:16:52.000000000 +0800 +++ sys/fs/msdosfs/msdosfs_vfsops.c 2007-09-25 23:48:07.000000000 +0800 @@ -268,6 +268,7 @@ return (EOPNOTSUPP); } if (!(pmp->pm_flags & MSDOSFSMNT_RONLY) && + !vfs_flagopt(mp->mnt_optnew, "noro", NULL, 0) && vfs_flagopt(mp->mnt_optnew, "ro", NULL, 0)) { error = VFS_SYNC(mp, MNT_WAIT, td); if (error) @@ -314,10 +315,12 @@ ro_to_rw = 1; } + if(!vfs_flagopt(mp->mnt_optnew, "noro", NULL, 0)) { vfs_flagopt(mp->mnt_optnew, "ro", &pmp->pm_flags, MSDOSFSMNT_RONLY); vfs_flagopt(mp->mnt_optnew, "ro", &mp->mnt_flag, MNT_RDONLY); + } if (ro_to_rw) { /* Now that the volume is modifiable, mark it dirty. */ @@ -417,7 +420,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();
Hi! Here is a version of last patch adjusted for 7.0-PRERELEASE: --- sys/fs/msdosfs/msdosfs_vfsops.c.orig 2007-08-16 01:40:09.000000000 +0800 +++ sys/fs/msdosfs/msdosfs_vfsops.c 2007-10-14 17:58:20.000000000 +0800 @@ -265,6 +265,7 @@ } } if (!(pmp->pm_flags & MSDOSFSMNT_RONLY) && + !vfs_flagopt(mp->mnt_optnew, "noro", NULL, 0) && vfs_flagopt(mp->mnt_optnew, "ro", NULL, 0)) { error = VFS_SYNC(mp, MNT_WAIT, td); if (error) @@ -314,10 +315,12 @@ ro_to_rw = 1; } + if(!vfs_flagopt(mp->mnt_optnew, "noro", NULL, 0)) { vfs_flagopt(mp->mnt_optnew, "ro", &pmp->pm_flags, MSDOSFSMNT_RONLY); vfs_flagopt(mp->mnt_optnew, "ro", &mp->mnt_flag, MNT_RDONLY); + } if (ro_to_rw) { /* Now that the volume is modifiable, mark it dirty. */
Hi! This problem has been fixed for RELENG_7 but is still present in 6.3-STABLE. Eugene Grosbein
State Changed From-To: open->patched Fixed in RELENG_7 but not in RELENG_6.
Responsible Changed From-To: freebsd-bugs->freebsd-fs Over to maintainer(s).
State Changed From-To: patched->closed This PR is fixed in head, 8.x and 7.x, but will not be merged to 6.x now that that branch is unsupported, sorry