quotactl bug: vfs_busy never unbusy-es

Chris Torek torek at torek.net
Thu Mar 10 02:07:37 UTC 2016


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).

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...

Chris


More information about the freebsd-fs mailing list