Date: Mon, 6 Jan 2003 04:06:20 +1100 (EST) From: Bruce Evans <bde@zeta.org.au> To: current@freebsd.org Cc: phk@freebsd.org Subject: specfs lock plumbing broken Message-ID: <20030106031002.N295-100000@gamplex.bde.org>
next in thread | raw e-mail | index | archive | help
The following change uncovers bugs in specfs locking and other places:
% RCS file: /home/ncvs/src/sys/ufs/ufs/ufs_vnops.c,v
% Working file: ufs_vnops.c
% head: 1.222
% ...
% ----------------------------
% revision 1.221
% date: 2003/01/04 08:47:19; author: phk; state: Exp; lines: +0 -9
% Since Jeffr made the std* functions the default in rev 1.63 of
% kern/vfs_defaults.c it is wrong for the individual filesystems to use
% the std* functions as that prevents override of the default.
%
% Found by: src/tools/tools/vop_table
% ----------------------------
specfs has always attempted to override the default to get no locking,
but having the std* in ufs_vnops.c overrode the override so specfs got
locking after all. This turns out to be essential for avoiding fatally
inconsistent lock states and not just for avoiding races. Without it,
the following bugs in ffs_vget() and deadfs are fatal:
- ffs_vget() returns a locked vnode ... according to ffs's idea of locking.
It calls lockmgr() directly, which gives the same result as ufs's
vop_lock which is vop_stdlock(). The vnode never gets unlocked if it
is handled by a file system whose vop_lock is vop_nolock (like specfs
with the above change).
- deadfs overrides the default for vop_lock but not for vop_unlock. So
when a vnode that was left bogusly locked by the above bugs is
revoked, deadfs_lock() is happy with it (it does nothing much), but
deadfs's vop_unlock (== vop_stdunlock) passes it to lockmgr() and
lockmgr() is often unhappy with it. For me, the bug was usually fatal
at reboot time because lockmgr() was unhappy with the shell unlocking
a vnode that was exclusively locked by a long-dead process. The
exclusive lock had no effect while the vnode was handled by specfs
because lockmgr() didn't get a chance to check it.
Fixing specfs is simple:
%%%
Index: spec_vnops.c
===================================================================
RCS file: /home/ncvs/src/sys/fs/specfs/spec_vnops.c,v
retrieving revision 1.193
diff -u -2 -r1.193 spec_vnops.c
--- spec_vnops.c 5 Jan 2003 10:03:57 -0000 1.193
+++ spec_vnops.c 5 Jan 2003 15:58:55 -0000
@@ -85,9 +89,7 @@
{ &vop_getwritemount_desc, (vop_t *) vop_stdgetwritemount },
{ &vop_ioctl_desc, (vop_t *) spec_ioctl },
- { &vop_islocked_desc, (vop_t *) vop_noislocked },
{ &vop_kqfilter_desc, (vop_t *) spec_kqfilter },
{ &vop_lease_desc, (vop_t *) vop_null },
{ &vop_link_desc, (vop_t *) vop_panic },
- { &vop_lock_desc, (vop_t *) vop_nolock },
{ &vop_mkdir_desc, (vop_t *) vop_panic },
{ &vop_mknod_desc, (vop_t *) vop_panic },
@@ -108,5 +110,4 @@
{ &vop_strategy_desc, (vop_t *) spec_strategy },
{ &vop_symlink_desc, (vop_t *) vop_panic },
- { &vop_unlock_desc, (vop_t *) vop_nounlock },
{ &vop_write_desc, (vop_t *) spec_write },
{ NULL, NULL }
%%%
Bugs found while investigating this:
- spec_print() is unreachable because ufs_vnops.c overrides it.
- spec_print() is of low quality: it doesn't print the device name or number.
- devfs_print() would be reachable but doesn't exist, so vprint() prints
even lower quality output for devfs since there nothing prints an inode
number either.
- the vop tables work even worse than might first appear.
- other entries in specfs's vop table seem to be unreachable or unnecessary
(because the default is better).
- deadfs is also missing an override for vop_islocked. This is OK provided
it is never passed locked vnodes.
Bruce
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-current" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20030106031002.N295-100000>