Re: [RFC PATCH] drm/nouveau: fix nested locking in mmap handler

[Thomas Hellstrom <thellstrom@vmware.com>]

On 09/24/2013 11:43 AM, Maarten Lankhorst wrote:
> Op 24-09-13 11:03, Thomas Hellstrom schreef:
>> On 09/24/2013 09:34 AM, Maarten Lankhorst wrote:
>>> Op 24-09-13 09:22, Thomas Hellstrom schreef:
>>>> Actually, it's not the locking order bo:reserve -> mmap_sem that triggers the lockdep warning, right? but the fact that copy_from_user could recurse into the fault handler? Grabbing bo:reseves recursively? which should leave us free to choose locking order at this point.
>>> Same thing.
>>>
>>> When PROVE_LOCKING is enabled the might_fault calls in copy_to/from_user do a fake locking of mmap_sem, which means all locking errors, provided that the reservation lock is taken at least once with mmap_sem held (eg the ttm fault handler is called at least once, it can be done separately, it doesn't need to be done in the same syscall). So any bugs will be found. The only thing to worry about is that lockdep turns off after finding 1 error, so you have to make sure it doesn't bomb out before completing tests, which is sometimes a problem on early rc kernels. ;)
>> My point was that when we only have copy_[to|from]_user_inatomic inside any bo:reservations, the might_fault would never be called inside any reservations and we should, in principle, be free to choose locking order, provided of course it could be done cleanly in fault()?
> I think it makes sense to keep mmap_sem the outer lock here, and not do scary things in fault. Especially if the mm locking is going to be changed in the future. But you're correct, if holding reservations only inatomic should be used.
>
> ~Maarten

Now that we after all were forced to enter the dark realm of copy-user 
slowpaths, I don't really have a strong opinion anymore, other than that 
we should try to avoid building too much code in that depends on either 
locking order, so that we could change it if _really_ needed.

/Thomas