From owner-freebsd-current Sun Jan 5 9: 6:12 2003 Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 9CB8337B401; Sun, 5 Jan 2003 09:06:09 -0800 (PST) Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16]) by mx1.FreeBSD.org (Postfix) with ESMTP id 1F51243EC2; Sun, 5 Jan 2003 09:06:08 -0800 (PST) (envelope-from bde@zeta.org.au) Received: from katana.zip.com.au (katana.zip.com.au [61.8.7.246]) by mailman.zeta.org.au (8.9.3/8.8.7) with ESMTP id EAA16380; Mon, 6 Jan 2003 04:06:00 +1100 Date: Mon, 6 Jan 2003 04:06:20 +1100 (EST) From: Bruce Evans X-X-Sender: bde@gamplex.bde.org To: current@freebsd.org Cc: phk@freebsd.org Subject: specfs lock plumbing broken Message-ID: <20030106031002.N295-100000@gamplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-current@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG 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