quotactl bug: vfs_busy never unbusy-es

[Chris Torek]

>Lets fix nullfs too.

And unionfs? :-)

>The uncomfortable issue with the interface is that
>it is not always possible to busy mount point after unbusy.

Yes.

>I propose to assume that for ENOENT error the busy ref is lost
>(this is the only possible error from vfs_busy()).

This seems pretty scary.  Also:

>I also verified that UFS quota code
>does not return this error for !quotaon case.

OK, but it certainly can for the quotaon case (though we're
protected by the fact that quotaon already had to do the unbusy
so it doesn't matter now).  Anyway, we are getting kind of
complicated here.

I think we can fix this with a fairly tiny KPI change.  Note that
the code currently only works for UFS.  Suppose we move the vfs_busy
to happen later (with potential failure, i.e., quotactl() call may
now return ENOENT because it can't busy the file system because
it's being unmounted -- this seems pretty harmless as it only
occurs in a race between quotactl() and unmount, and the caller
could have lost that race anyway).

I'm not 100% sure this is correct, but it seems a bit less
scary to me :-)  But note that it is COMPLETELY untested (not
even compiled), I'm just seeing if you think this is a reasonable
approach.  Basically, we're just moving the vfs_busy/unbusy into
UFS itself, and permitting it to fail at that point (for all
ops, not just Q_QUOTAON).

(Later we can refactor all this to be usable from, e.g., tmpfs.)

Chris

diff --git a/sys/kern/vfs_syscalls.c b/sys/kern/vfs_syscalls.c
index 11813fc..e1ab670 100644
--- a/sys/kern/vfs_syscalls.c
+++ b/sys/kern/vfs_syscalls.c
@@ -173,6 +173,11 @@ sys_quotactl(td, uap)
 	AUDIT_ARG_UID(uap->uid);
 	if (!prison_allow(td->td_ucred, PR_ALLOW_QUOTAS))
 		return (EPERM);
+	/*
+	 * Reference the mount point so that it will remain in core
+	 * across the VFS_QUOTACTL call.  We used to vfs_busy() it
+	 * here, but now we leave that to the underlying file system.
+	 */
 	NDINIT(&nd, LOOKUP, FOLLOW | LOCKLEAF | AUDITVNODE1, UIO_USERSPACE,
 	    uap->path, td);
 	if ((error = namei(&nd)) != 0)
@@ -181,25 +186,8 @@ sys_quotactl(td, uap)
 	mp = nd.ni_vp->v_mount;
 	vfs_ref(mp);
 	vput(nd.ni_vp);
-	error = vfs_busy(mp, 0);
-	vfs_rel(mp);
-	if (error != 0)
-		return (error);
 	error = VFS_QUOTACTL(mp, uap->cmd, uap->uid, uap->arg);
-
-	/*
-	 * Since quota on operation typically needs to open quota
-	 * file, the Q_QUOTAON handler needs to unbusy the mount point
-	 * before calling into namei.  Otherwise, unmount might be
-	 * started between two vfs_busy() invocations (first is our,
-	 * second is from mount point cross-walk code in lookup()),
-	 * causing deadlock.
-	 *
-	 * Require that Q_QUOTAON handles the vfs_busy() reference on
-	 * its own, always returning with ubusied mount point.
-	 */
-	if ((uap->cmd >> SUBCMDSHIFT) != Q_QUOTAON)
-		vfs_unbusy(mp);
+	vfs_rel(mp);
 	return (error);
 }
 
diff --git a/sys/ufs/ufs/ufs_vfsops.c b/sys/ufs/ufs/ufs_vfsops.c
index 5bb73ea..fc5f5bb 100644
--- a/sys/ufs/ufs/ufs_vfsops.c
+++ b/sys/ufs/ufs/ufs_vfsops.c
@@ -92,9 +92,6 @@ ufs_quotactl(mp, cmds, id, arg)
 	void *arg;
 {
 #ifndef QUOTA
-	if ((cmds >> SUBCMDSHIFT) == Q_QUOTAON)
-		vfs_unbusy(mp);
-
 	return (EOPNOTSUPP);
 #else
 	struct thread *td;
@@ -115,21 +112,24 @@ ufs_quotactl(mp, cmds, id, arg)
 			break;
 
 		default:
-			if (cmd == Q_QUOTAON)
-				vfs_unbusy(mp);
 			return (EINVAL);
 		}
 	}
 	if ((u_int)type >= MAXQUOTAS) {
-		if (cmd == Q_QUOTAON)
-			vfs_unbusy(mp);
 		return (EINVAL);
 	}
 
+	/*
+	 * Make sure we're not unmounting.
+	 */
+	error = vfs_busy(mp);
+	if (error)
+		return (error);
+
 	switch (cmd) {
 	case Q_QUOTAON:
 		error = quotaon(td, mp, type, arg);
-		break;
+		goto done;	/* quotaon does vfs_unbusy itself */
 
 	case Q_QUOTAOFF:
 		error = quotaoff(td, mp, type);
@@ -171,6 +171,8 @@ ufs_quotactl(mp, cmds, id, arg)
 		error = EINVAL;
 		break;
 	}
+	vfs_unbusy(mp);
+done:
 	return (error);
 #endif
 }