Date: Wed, 09 Mar 2016 18:07:09 -0800 From: Chris Torek <torek@torek.net> To: freebsd-fs@freebsd.org Subject: quotactl bug: vfs_busy never unbusy-es Message-ID: <201603100207.u2A279cC045486@elf.torek.net>
next in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201603100207.u2A279cC045486>