Date: Thu, 12 Jul 2007 11:41:15 +0300 From: Kostik Belousov <kostikbel@gmail.com> To: Bruce Evans <brde@optusnet.com.au> Cc: bugs@freebsd.org, fs@freebsd.org Subject: Re: msdosfs not MPSAFE Message-ID: <20070712084115.GA2200@deviant.kiev.zoral.com.ua> In-Reply-To: <20070710233455.O2101@besplex.bde.org> References: <20070710233455.O2101@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070712084115.GA2200>