msdosfs not MPSAFE

[Kostik Belousov]

On Mon, Jul 16, 2007 at 08:18:14PM +1000, Bruce Evans wrote:
> On Thu, 12 Jul 2007, Kostik Belousov wrote:
> 
> >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 
> >>>>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.
> 
> I think msdosfs_lookup() doesn't need to own nambuf near the deget()
> call.  Not sure -- I was looking more at msdosfs_readdir().
> 
> >>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.
> 
> 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.
> 
> sx xlocking works, but is not quite right:
> %  /*
> % + * XXX msdosfs_lookup() is split up because unlocking before all the 
> 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 = 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 ?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-fs/attachments/20070721/e18cc3d9/attachment.pgp