[patch] nmount ro, rw and negated option handling

[Jaakko Heinonen]

On 2011-01-19, Jaakko Heinonen wrote:
> On 2011-01-19, Craig Rodrigues wrote:
> > I disagree with your patch and do not approve it.
>  
> > > 1. Have mountd(8) running.
> > > 2. # mdconfig -a -t vnode -f ufsimg
> > > 3. # mount -o ro,rw /dev/md0 /mnt
> 
> With your patch[1] after the third step the mount point has both "ro"
> and and "noro" active but the MNT_RDONLY flag is not set. Again, you
> will eventually get the  "ffs_sync: rofs mod" (or similar) panic because
> the "ro" option is active during remount.

Could you please elaborate on why my patch isn't acceptable and/or
suggest an alternative approach to fix the bugs. As I said earlier the
patch you posted doesn't solve the issues. I really want to get these
bugs fixed.

One option would be to revert the file system code to use the MNT_RDONLY
flag instead of checking for the "ro" string option until nmount gets
fixed but I don't think it's feasible because too many file systems are
already testing for "ro".

A quick fix for the particular problem above would be to commit the
vfs_equalopts() part of my patch. I believe that the change is correct
as the code stands now. It is not enough to fix PR kern/150206.

Another related bug is in vfs_domount_update(): if VFS_MOUNT() succeeds
but vfs_export() fails, old mount flags and options are restored. I
think this shouldn't happen when VFS_MOUNT() has been already
successfully completed and this is the final reason why FFS fs_ronly and
the MNT_RDONLY flag get out of sync. Does the patch below look sane?

%%%
Index: sys/kern/vfs_mount.c
===================================================================
--- sys/kern/vfs_mount.c	(revision 217792)
+++ sys/kern/vfs_mount.c	(working copy)
@@ -683,8 +683,6 @@ bail:
 		}
 	}
 
-	if (error != 0)
-		vfs_freeopts(optlist);
 	return (error);
 }
 
@@ -800,6 +798,7 @@ vfs_domount_first(
 	}
 	if (error != 0) {
 		vput(vp);
+		vfs_freeopts(fsdata);
 		return (error);
 	}
 	VOP_UNLOCK(vp, 0);
@@ -824,6 +823,7 @@ vfs_domount_first(
 		vp->v_iflag &= ~VI_MOUNT;
 		VI_UNLOCK(vp);
 		vrele(vp);
+		vfs_freeopts(fsdata);
 		return (error);
 	}
 
@@ -881,15 +881,15 @@ vfs_domount_update(
 	struct oexport_args oexport;
 	struct export_args export;
 	struct mount *mp;
-	int error, flag;
+	int error, export_error, flag;
 
 	mtx_assert(&Giant, MA_OWNED);
 	ASSERT_VOP_ELOCKED(vp, __func__);
 	KASSERT((fsflags & MNT_UPDATE) != 0, ("MNT_UPDATE should be here"));
 
 	if ((vp->v_vflag & VV_ROOT) == 0) {
-		vput(vp);
-		return (EINVAL);
+		error = EINVAL;
+		goto fail;
 	}
 	mp = vp->v_mount;
 	/*
@@ -898,28 +898,26 @@ vfs_domount_update(
 	 */
 	flag = mp->mnt_flag;
 	if ((fsflags & MNT_RELOAD) != 0 && (flag & MNT_RDONLY) == 0) {
-		vput(vp);
-		return (EOPNOTSUPP);	/* Needs translation */
+		error = EOPNOTSUPP;	/* Needs translation */
+		goto fail;
 	}
 	/*
 	 * Only privileged root, or (if MNT_USER is set) the user that
 	 * did the original mount is permitted to update it.
 	 */
 	error = vfs_suser(mp, td);
-	if (error != 0) {
-		vput(vp);
-		return (error);
-	}
+	if (error != 0)
+		goto fail;
 	if (vfs_busy(mp, MBF_NOWAIT)) {
-		vput(vp);
-		return (EBUSY);
+		error = EBUSY;
+		goto fail;
 	}
 	VI_LOCK(vp);
 	if ((vp->v_iflag & VI_MOUNT) != 0 || vp->v_mountedhere != NULL) {
 		VI_UNLOCK(vp);
 		vfs_unbusy(mp);
-		vput(vp);
-		return (EBUSY);
+		error = EBUSY;
+		goto fail;
 	}
 	vp->v_iflag |= VI_MOUNT;
 	VI_UNLOCK(vp);
@@ -942,11 +940,12 @@ vfs_domount_update(
 	 */
 	error = VFS_MOUNT(mp);
 
+	export_error = 0;
 	if (error == 0) {
 		/* Process the export option. */
 		if (vfs_copyopt(mp->mnt_optnew, "export", &export,
 		    sizeof(export)) == 0) {
-			error = vfs_export(mp, &export);
+			export_error = vfs_export(mp, &export);
 		} else if (vfs_copyopt(mp->mnt_optnew, "export", &oexport,
 		    sizeof(oexport)) == 0) {
 			export.ex_flags = oexport.ex_flags;
@@ -958,7 +957,7 @@ vfs_domount_update(
 			export.ex_masklen = oexport.ex_masklen;
 			export.ex_indexfile = oexport.ex_indexfile;
 			export.ex_numsecflavors = 0;
-			error = vfs_export(mp, &export);
+			export_error = vfs_export(mp, &export);
 		}
 	}
 
@@ -1005,6 +1004,11 @@ end:
 	vp->v_iflag &= ~VI_MOUNT;
 	VI_UNLOCK(vp);
 	vrele(vp);
+	return (error != 0 ? error : export_error);
+
+fail:
+	vput(vp);
+	vfs_freeopts(fsdata);
 	return (error);
 }
 
%%%

-- 
Jaakko