Possible lock inversion in ttm_bo_vm_access

[Koenig, Christian]

Am 15.10.2018 um 12:42 schrieb Thomas Hellstrom:
> On 10/15/2018 10:43 AM, Koenig, Christian wrote:
>> Hi Thomas,
>>
>>> 1. You need the a lock with the order lock->mmap_sem, cause otherwise
>>> you run into trouble when drm_vma_node_unmap() needs to be called.
>>> Why is this? unmap_mapping_range() never takes the mmap_sem. It takes
>>> a finer-level per-address-space lock.
>> Well, that is interesting. Where do we actually then have the order of
>> reservation->mmap_sem defined?
>
> TTM doesn't. But some drivers using TTM do, which prompted the wording 
> in the fault handler comments.
> But as Daniel mentioned, to be able to share buffers throughout DRM, 
> we at some point need to agree on a locking order, and when we do it's 
> important that we consider all relevant use-cases and workarounds.
>
> To me it certainly looks like the vm_ops implementations would be more 
> straightforward if we were to use mmap_sem->bo_reserve, but that would 
> again require a number of drivers to rework their copy_*_user code.
> If those drivers, however, insisted on keeping their copy_*_user code, 
> they probably need to fix the recursive bo reservation up anyway, for 
> example by making sure not to copy from a vma that has VM_IO set.

Yeah, completely agree. If you ask me using copy_(from|to)_user while a 
BO is reserved is currently completely forbidden.

[SNIP]
>> We need one lock which is held while the BO backing store is replaced to
>> do faults and other stuff.
>>
>> And we need another lock which is held while we allocate backing store
>> for the BO, do command submission and prepare moves.
>>
>> As far as I can see the first one should be a simple mutex while the
>> second is the reservation object.
>
> So why can't we use reservation for both, like today?

We are going to need to sooner or later anyway for recoverable GPU 
faults. E.g. in such a handler you only need the current location of a 
BO and not add fences or move it around.

Additional to that copy_(from|to)_user can definitely take the mmap_sem 
in some code paths.

So if we really want to allow that while BOs are reserved we need to 
split up the lock anyway.

Regards,
Christian.