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