Date: Sat, 21 Jul 2007 09:34:34 +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: <20070721063434.GI2200@deviant.kiev.zoral.com.ua> In-Reply-To: <20070716195556.P12807@besplex.bde.org> References: <20070710233455.O2101@besplex.bde.org> <20070712084115.GA2200@deviant.kiev.zoral.com.ua> <20070712225324.F9515@besplex.bde.org> <20070712142127.GD2200@deviant.kiev.zoral.com.ua> <20070716195556.P12807@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--12mfjY/2IcLDkfPj
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Mon, Jul 16, 2007 at 08:18:14PM +1000, Bruce Evans wrote:
> On Thu, 12 Jul 2007, Kostik Belousov wrote:
>=20
> >On Thu, Jul 12, 2007 at 11:33:40PM +1000, Bruce Evans wrote:
> >>
> >>On Thu, 12 Jul 2007, Kostik Belousov wrote:
> >>
> >>>On Wed, Jul 11, 2007 at 12:08:19AM +1000, Bruce Evans wrote:
> >>>>msdsofs has been broken since Giant locking for file systems (or=20
> >>>>syscalls)
> >>>>was removed. It allows multiple threads to race accessing the shared
> >>>>static buffer `nambuf' and related variables. This causes remarkably
> >>
> >>>It seems that msdosfs_lookup() can sleep, thus Giant protection would =
be
> >>>lost.
> >>
> >>It can certainly block in bread().
> >Besides bread(), there is a (re)locking for ".." case, and deget() call,
> >that itself calls malloc(M_WAITOK), vfs_hash_get(), getnewvnode() and
> >readep(). The latter itself calls bread().
> >
> >This is from the brief look.
>=20
> I think msdosfs_lookup() doesn't need to own nambuf near the deget()
> call. Not sure -- I was looking more at msdosfs_readdir().
>=20
> >>How does my adding Giant locking help? I checked that at least in
> >>FreeBSD-~5.2-current, msdosfs_readdir() is already Giant-locked, so my
> >>fix just increments the recursion count. What happens to recursively-
> >>held Giant locks across sleeps? I think they should cause a KASSERT()
> >>failure, but if they are handled by only dropping Giant once then my
> >>fix might sort of work but sleeps would be broken generally.
> >>
> >Look at the kern/kern_sync.c:_sleep(). It does DROP_GIANT(), that (from
> >the sys/mutex.h) calls mtx_unlock() until Giant is owned.
>=20
> So it is very mysterious that Giant locking helped. Anyway, it doesn't
> work, and cases where it doesn't help showed up in further testing.
>=20
> sx xlocking works, but is not quite right:
> % /*
> % + * XXX msdosfs_lookup() is split up because unlocking before all the=
=20
> returns
> % + * in the original function would be too churning.
> % + */
> % +int
> % +msdosfs_lookup(ap)
> % + struct vop_cachedlookup_args *ap;
> % +{
> % + int error;
> % +
> % + sx_xlock(&mbnambuf_lock);
> % + error =3D msdosfs_lookup_locked(ap);
> % + sx_xunlock(&mbnambuf_lock);
> % + return (error);
> % +}
> % +
> % +/*
Assume that a directory A is participating in lookup() from two threads:
thread 1 lookup the A itself;
thread 2 lookup some entry in the A.
Then, thread 1 would have mbnambuf_lock locked, and may wait for A'
vnode lock;
thread 2 shall own vnode lock for A, then locking mbnambuf_lock.
I do not see what may prevent this LOR scenario from realizing, or what
make it harmless.
Did I missed something ?
--12mfjY/2IcLDkfPj
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (FreeBSD)
iD8DBQFGoaj4C3+MBN1Mb4gRAlcwAKCbJh5IOqLlZkd05jW2t6ktgbIaQACgnJNr
dtX3BEnDUbOzxeOKkxXrmW8=
=ts/I
-----END PGP SIGNATURE-----
--12mfjY/2IcLDkfPj--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070721063434.GI2200>