From owner-freebsd-bugs@FreeBSD.ORG Wed Jul 11 20:20:03 2007 Return-Path: X-Original-To: bugs@freebsd.org Delivered-To: freebsd-bugs@FreeBSD.ORG Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 1E87916A469; Wed, 11 Jul 2007 20:20:03 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from fallbackmx03.syd.optusnet.com.au (fallbackmx03.syd.optusnet.com.au [211.29.133.136]) by mx1.freebsd.org (Postfix) with ESMTP id A749113C4D1; Wed, 11 Jul 2007 20:20:02 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail07.syd.optusnet.com.au (mail07.syd.optusnet.com.au [211.29.132.188]) by fallbackmx03.syd.optusnet.com.au (8.12.11.20060308/8.12.11) with ESMTP id l6AE8OCB008565; Wed, 11 Jul 2007 00:08:24 +1000 Received: from besplex.bde.org (c220-239-235-248.carlnfd3.nsw.optusnet.com.au [220.239.235.248]) by mail07.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id l6AE8JSj012079 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 11 Jul 2007 00:08:22 +1000 Date: Wed, 11 Jul 2007 00:08:19 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: fs@freebsd.org Message-ID: <20070710233455.O2101@besplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: bugs@freebsd.org Subject: msdosfs not MPSAFE X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 11 Jul 2007 20:20:03 -0000 msdsofs has been broken since Giant locking for file systems (or syscalls) was removed. It allows multiple threads to race accessing the shared static buffer `nambuf' and related variables. This causes remarkably few problems. One case that breaks reliably is concurrent tars or finds, especially with a small cluster size, even on a UP system. Then getdirentries() returns garbage entries and/or lookup() of correct entries can't find things. On my UP system, I think the race occurs mainly when getdirentries() blocks on i/o and a directory entry spans the block boundary. Then another getdirentries() or lookup() can run, and if it does then it will almost certainly corrupt the state for the blocked getdirentries(). On SMP systems, I think the race occurs much more often and suspect that it is more harmful. Quick fix for an old version of FreeBSD. The part in msdosfs_lookup.c has not been tested at runtime. The part in msdosfs_vnops.c may also fix bugs involving cookie handling (but not a memory leak like I first thought?). msdosfs_lookup.c is harder to fix because it has no common cleanup code. %%% Index: msdosfs_lookup.c =================================================================== RCS file: /home/ncvs/src/sys/fs/msdosfs/msdosfs_lookup.c,v retrieving revision 1.40 diff -u -2 -r1.40 msdosfs_lookup.c --- msdosfs_lookup.c 26 Dec 2003 17:24:37 -0000 1.40 +++ msdosfs_lookup.c 10 Jul 2007 13:33:37 -0000 @@ -78,11 +78,11 @@ * memory denode's will be in synch. */ -int -msdosfs_lookup(ap) +static int +msdosfs_lookup_locked( struct vop_cachedlookup_args /* { struct vnode *a_dvp; struct vnode **a_vpp; struct componentname *a_cnp; - } */ *ap; + } */ *ap) { struct vnode *vdp = ap->a_dvp; @@ -560,4 +561,20 @@ /* + * XXX msdosfs_lookup() is split up because unlocking Giant before all the + * returns in the original function would be too churning. + */ +int +msdosfs_lookup(ap) + struct vop_cachedlookup_args *ap; +{ + int error; + + mtx_lock(&Giant); /* XXX for exclusive access to nambuf. */ + error = msdosfs_lookup_locked(ap); + mtx_unlock(&Giant); + return (error); +} + +/* * dep - directory entry to copy into the directory * ddep - directory to add to Index: msdosfs_vnops.c =================================================================== RCS file: /home/ncvs/src/sys/fs/msdosfs/msdosfs_vnops.c,v retrieving revision 1.147 diff -u -2 -r1.147 msdosfs_vnops.c --- msdosfs_vnops.c 4 Feb 2004 21:52:53 -0000 1.147 +++ msdosfs_vnops.c 10 Jul 2007 05:08:17 -0000 @@ -1559,4 +1592,5 @@ } + mtx_lock(&Giant); /* XXX for exclusive access to nambuf. */ mbnambuf_init(); off = offset; @@ -1575,10 +1609,13 @@ if (error) { brelse(bp); - return (error); + Debugger("used to leak cookies 1"); + break; } n = min(n, blsize - bp->b_resid); if (n == 0) { brelse(bp); - return (EIO); + error = EIO; + Debugger("used to leak cookies 2"); + break; } @@ -1687,4 +1725,6 @@ } out: + mtx_unlock(&Giant); + /* Subtract unused cookies */ if (ap->a_ncookies) %%% Please fix this better. mbnambuf_init() could return a non-static buffer that doesn't require locking. Deallocation would be painful. Note that even the details for Giant locking can't be hidden in msdosfs_conv.c like the current interfaces intend, since mbnambuf_init() is used a lot to reinitialize an in-use buffer, and there is no interface to drop usage. Bruce