msdosfs not MPSAFE

[Kostik Belousov]

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
> 
> >>[Add Giant locking]
> 
> >>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.
> 
> >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 now think this is not really an SMP bug.  The nambuf* data structure
> requires a long-term global lock, but it has never had one.  The bug
> seems to be relatively new.  nambuf* is for multi-byte characters, not
> for long names, and has only existed since 2003 (msdosfs_vnops.c 1.141,
> etc.).  I thought that long names were built up in nambuf, but they
> are apparently built up in the directory entry.  This should work for
> multi-byte characters too -- don't translate anything until all the low-
> level directory entries have been accumulated.
> 
> 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.
-------------- 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/20070712/db87a89d/attachment-0001.pgp