Possible lock inversion in ttm_bo_vm_access

[Thomas Hellstrom]

On 10/15/2018 04:47 PM, Daniel Vetter wrote:
> On Mon, Oct 15, 2018 at 2:30 PM Thomas Hellstrom <thomas at shipmail.org> wrote:
>> On 10/15/2018 12:55 PM, Koenig, Christian wrote:
>>> 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.
>> So if we all agree that this should be completely forbidden, and this is
>> what requires the bo_reserve->mmap_sem locking order, then I guess the
>> logical follow up is trying to enforce mmap_sem->bo_reserve and pursuade
>> driver maintainers to convert. Perhaps as a configurable lockdep option?
> The problem with adding that under e.g. CONFIG_PROVE_LOCKING or
> similar is that we have drivers violating it right now. And last time
> around this entire discussion came up no one wanted to do the work to
> convert drivers, since it means you get to add a slowpath to the most
> complex ioctl they have.
> -Daniel

Hmm. I actually had something like CONFIG_DRM_PROVE_LOCKING in mind; To 
be enabled only for DRM developers.

But regardless, that slowpath is not hit frequently enough to affect 
real-life performance, right? So it's more a matter of coding effort 
rather than valid performance concerns?

/Thomas