Re: drm_vm.c:drm_mmap: possible circular locking dependency detected (was: Re: Linux 2.6.33-rc2 - Merry Christmas ...)

[Linus Torvalds <torvalds@linux-foundation.org>]

On Mon, 28 Dec 2009, KOSAKI Motohiro wrote:
> >        [<ffffffff810694c0>] __lock_acquire+0x1373/0x16fd
> >        [<ffffffff8106993c>] lock_acquire+0xf2/0x116
> >        [<ffffffff810bb2b5>] might_fault+0x95/0xb8			<- mmap_sem
> >        [<ffffffff810e87d6>] filldir+0x75/0xd0				<- sysfs_mutex
> >        [<ffffffff8112be2a>] sysfs_readdir+0x10f/0x149
> 
> This output seems to suggest need to fix drm.

Actually, this painful dependency may technically be a bug in drm, but at 
the same time, it's really filldir() that makes it _very_ hard for certain 
locking scenarios because it has that callback to the VFS layer that takes 
the mmap_sem, and sysfs itself wants the sysfs_mutex in paths where it is 
not at all unreasonable to hold the mmap_sem.

We've seen it several times (yes, mostly with drm, but it's been seen with 
others too), and it's very annoying. It can be fixed by having very 
careful readdir implementations, but I really would blame sysfs in 
particular for having a very annoying lock reversal issue when used 
reasonably.

So the optimal situation would be for sysfs to not have that annoying lock 
dependency, and it would really have to be sysfs_readdir() that drops the 
sysfs_mutex around the filldir call (and that obviously implies having to 
re-validate and be really careful).

Added Eric and Greg to the cc, in case the sysfs people want to solve it.

The other alternative would be to fix this on a VFS layer by changing how 
'readdir/filldir' interacts, and instead make it fill in a kernel buffer - 
avoiding the mmap_sem issue entirely. And than later (in readdir) that 
kernel buffer could be copied to user space without holding any other 
locks.

I like the VFS approach because I think we could possibly use that as a 
first approach to eventually try to think about caching readdir() results 
at a VFS level - readdir() is currently the _only_ main filesystem 
callback that always calls into the low-level filesystem, and always takes 
a lot of locks. I'm adding Al to the Cc for that - he knows about this 
issue from me previously thinking aloud along these lines.

And yes, one option would be to just fix drm - by avoiding calling any 
sysfs functions while holding the mmap_lock (either in the mmap callback 
or the page fault paths). However, as mentioned, I really do think that 
the blame can be laid on sysfs for trying to be a nice generic interface, 
but having a damn inconvenient locking model.

			Linus