quotactl bug: vfs_busy never unbusy-es

[Konstantin Belousov]

On Wed, Mar 09, 2016 at 06:07:09PM -0800, Chris Torek wrote:
> This bug is fairly small, as it mainly just prevents file system
> unmont of non-UFS file systems (including devfs and tmpfs as well
> as real ones like zfs) after issuing a particular quotactl() system
> call (which must be done as root) on those non-UFS file systems:
> 
> 	rc = quotactl(path, QCMD(Q_QUOTAON, 0), 0, NULL);
> 
> No user-land utility will actually do the evil system call (the
> main usage of quotactl() is in libutil, which skips over any
> non-ufs file system, it has literal strcmp()s in it for this).
> Still, it's just kind of sloppy at best.
> 
> The bug occurs because ufs_quotactl has a special case, which
> sys_quotactl provides for, but no other file system implements
> this special case.  (I checked them all: nullfs and unionfs pass
> the op down, but that doesn't help; smbfs has its own private copy
> of vfs_stdquotactl; and vfs_stdquotactl doesn't do it.  This also
> means that nullfs and unionfs are actually worse than the others,
> if they happen to have a UFS file system in their pass-through: on
> unionfs you could get a panic, I think, when UFS attempts the
> vfs_unbusy() on the lower layer.)
> 
> Here's the bit in sys_quotactl() that causes problems:
> 
> 	error = vfs_busy(mp, 0);
> 	vfs_rel(mp);
> 	if (error != 0)
> 		return (error);
> 	error = VFS_QUOTACTL(mp, uap->cmd, uap->uid, uap->arg);
> 	/* large comment here about why the next two lines */
> 	if ((uap->cmd >> SUBCMDSHIFT) != Q_QUOTAON)
> 		vfs_unbusy(mp);
> 
> Now, ufs_quotactl() does in fact do the special unbusy dance,
> but nobody else does.
> 
> Since quotas only really work on UFS there are a number of paths
> by which we might fix this.  Maybe the best would be to change the
> system call: for quotaon() ops, perhaps the caller should pass the
> open file descriptors, for instance.  Avoiding the whole
> busy/unbusy sequence here would be good (file systems that
> actually implement quotas, i.e., UFS, would do their own locking
> as needed).
Yes, this is a possible route, although the change would break VFS KBI.

> 
> Another option is to change the type-signature of VFS_QUOTACTL:
> pass in a pointer to a flags, and have the callee (UFS) set or
> clear a flag to say "I did the unbusy".  Naive callees, including
> all the error-returning ones, need do nothing.  But this still
> means that nullfs and unionfs need work, as they don't actually
> do any vfs_busy/unbusy on their underlying mount points.
> 
> I'm not sure why quotaon() is designed to do its own path lookup,
> but if there's no real reason to require that, I'd try the first
> idea.
> 
> Meanwhile, I'm open to other suggestions...

Quotaon operates on some file which possibly does not belong to the
mount point where the quotas are enabled.  In fact, this setup is
easier for UFS, since metadata updates do not trigger data writes.
So the lookup in the UFS quotaon is unrelated to the lookup in the
syscall.  Also, it was considered that other filesystems could not
need such split.

The special casing for quotaon() is there because we might be unable to
vfs_busy() the mount point, if parallel unmount is in progress.  So
sys_quotactl() must be ready to handle the case where VFS method
lost vfs_busy() reference.  I tried to fix this and did not thought
about other filesystems.

Why not do just the following ?

With regard to the ufs/ufs/quota.h pollution of the VFS code, this
is ugly, I agree. I wanted to move the ufs quota code into vfs layer
for very long time. UFS quota has no UFS specific code at all, and is
immediately reusable for other filesystems. E.g., I see tmpfs could
immediately benefit from the shared quota code.

diff --git a/sys/kern/vfs_default.c b/sys/kern/vfs_default.c
index a7977bf..26e45f4 100644
--- a/sys/kern/vfs_default.c
+++ b/sys/kern/vfs_default.c
@@ -66,6 +66,8 @@ __FBSDID("$FreeBSD$");
 #include <vm/vm_pager.h>
 #include <vm/vnode_pager.h>
 
+#include <ufs/ufs/quota.h>
+
 static int	vop_nolookup(struct vop_lookup_args *);
 static int	vop_norename(struct vop_rename_args *);
 static int	vop_nostrategy(struct vop_strategy_args *);
@@ -1190,6 +1192,8 @@ vfs_stdquotactl (mp, cmds, uid, arg)
 	void *arg;
 {
 
+	if ((cmds >> SUBCMDSHIFT) == Q_QUOTAON)
+		vfs_unbusy(mp);
 	return (EOPNOTSUPP);
 }