From owner-freebsd-bugs@FreeBSD.ORG Thu Jul 12 08:56:34 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 E313016A46B for ; Thu, 12 Jul 2007 08:56:34 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from relay02.kiev.sovam.com (relay02.kiev.sovam.com [62.64.120.197]) by mx1.freebsd.org (Postfix) with ESMTP id 29D7113C4BC for ; Thu, 12 Jul 2007 08:56:34 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from [89.162.146.170] (helo=skuns.kiev.zoral.com.ua) by relay02.kiev.sovam.com with esmtps (TLSv1:AES256-SHA:256) (Exim 4.67) (envelope-from ) id 1I8uEo-0002g2-Sj; Thu, 12 Jul 2007 11:41:30 +0300 Received: from deviant.kiev.zoral.com.ua (root@[10.1.1.148]) by skuns.kiev.zoral.com.ua (8.14.1/8.14.1) with ESMTP id l6C8fGpL088945 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 12 Jul 2007 11:41:16 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: from deviant.kiev.zoral.com.ua (kostik@localhost [127.0.0.1]) by deviant.kiev.zoral.com.ua (8.14.1/8.14.1) with ESMTP id l6C8fGIk056169; Thu, 12 Jul 2007 11:41:16 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: (from kostik@localhost) by deviant.kiev.zoral.com.ua (8.14.1/8.14.1/Submit) id l6C8fFLf056168; Thu, 12 Jul 2007 11:41:15 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: deviant.kiev.zoral.com.ua: kostik set sender to kostikbel@gmail.com using -f Date: Thu, 12 Jul 2007 11:41:15 +0300 From: Kostik Belousov To: Bruce Evans Message-ID: <20070712084115.GA2200@deviant.kiev.zoral.com.ua> References: <20070710233455.O2101@besplex.bde.org> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="adlATi0AGRzVk0TM" Content-Disposition: inline In-Reply-To: <20070710233455.O2101@besplex.bde.org> User-Agent: Mutt/1.4.2.3i X-Virus-Scanned: ClamAV version 0.90.3, clamav-milter version 0.90.3 on skuns.kiev.zoral.com.ua X-Virus-Status: Clean X-Spam-Status: No, score=0.6 required=5.0 tests=ALL_TRUSTED, DNS_FROM_AHBL_RHSBL autolearn=no version=3.2.1 X-Spam-Checker-Version: SpamAssassin 3.2.1 (2007-05-02) on skuns.kiev.zoral.com.ua X-Scanner-Signature: 0c76fa1f3f831c98d9dd14672e80d09a X-DrWeb-checked: yes X-SpamTest-Envelope-From: kostikbel@gmail.com X-SpamTest-Group-ID: 00000000 X-SpamTest-Header: Not Detected X-SpamTest-Info: Profiles 1209 [July 11 2007] X-SpamTest-Info: helo_type=3 X-SpamTest-Method: none X-SpamTest-Rate: 0 X-SpamTest-Status: Not detected X-SpamTest-Status-Extended: not_detected X-SpamTest-Version: SMTP-Filter Version 3.0.0 [0255], KAS30/Release Cc: bugs@freebsd.org, fs@freebsd.org Subject: Re: 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: Thu, 12 Jul 2007 08:56:35 -0000 --adlATi0AGRzVk0TM Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jul 11, 2007 at 12:08:19AM +1000, Bruce Evans wrote: > 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. >=20 > 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. >=20 > %%% > Index: msdosfs_lookup.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > 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 =3D ap->a_dvp; > @@ -560,4 +561,20 @@ >=20 > /* > + * XXX msdosfs_lookup() is split up because unlocking Giant before all t= he > + * 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 =3D 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 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > 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 @@ > } >=20 > + mtx_lock(&Giant); /* XXX for exclusive access to nambuf. */ > mbnambuf_init(); > off =3D offset; > @@ -1575,10 +1609,13 @@ > if (error) { > brelse(bp); > - return (error); > + Debugger("used to leak cookies 1"); > + break; > } > n =3D min(n, blsize - bp->b_resid); > if (n =3D=3D 0) { > brelse(bp); > - return (EIO); > + error =3D EIO; > + Debugger("used to leak cookies 2"); > + break; > } >=20 > @@ -1687,4 +1725,6 @@ > } > out: > + mtx_unlock(&Giant); > + > /* Subtract unused cookies */ > if (ap->a_ncookies) > %%% >=20 > 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. >=20 > Bruce It seems that msdosfs_lookup() can sleep, thus Giant protection would be lost. --adlATi0AGRzVk0TM Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.7 (FreeBSD) iD8DBQFGlekrC3+MBN1Mb4gRApEAAJ94vAGI9bvng4lZh89Nfj2G1xeI3ACfRaNx gmMuH2O7CCdU5K1DFVRwYgw= =0cKT -----END PGP SIGNATURE----- --adlATi0AGRzVk0TM--