Date: Wed, 11 Jul 2007 00:08:19 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: fs@freebsd.org Cc: bugs@freebsd.org Subject: msdosfs not MPSAFE Message-ID: <20070710233455.O2101@besplex.bde.org>
next in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070710233455.O2101>