[Linaro-mm-sig] [PATCH 1/5] dma-buf: add optional invalidate_mappings callback v2

[Christian König]

Am 20.03.2018 um 08:44 schrieb Daniel Vetter:
> On Mon, Mar 19, 2018 at 5:23 PM, Christian König
> <ckoenig.leichtzumerken at gmail.com> wrote:
>> Am 19.03.2018 um 16:53 schrieb Chris Wilson:
>>> Quoting Christian König (2018-03-16 14:22:32)
>>> [snip, probably lost too must context]
>>>> This allows for full grown pipelining, e.g. the exporter can say I need
>>>> to move the buffer for some operation. Then let the move operation wait
>>>> for all existing fences in the reservation object and install the fence
>>>> of the move operation as exclusive fence.
>>> Ok, the situation I have in mind is the non-pipelined case: revoking
>>> dma-buf for mmu_invalidate_range or shrink_slab. I would need a
>>> completion event that can be waited on the cpu for all the invalidate
>>> callbacks. (Essentially an atomic_t counter plus struct completion; a
>>> lighter version of dma_fence, I wonder where I've seen that before ;)
>>
>> Actually that is harmless.
>>
>> When you need to unmap a DMA-buf because of mmu_invalidate_range or
>> shrink_slab you need to wait for it's reservation object anyway.
> reservation_object only prevents adding new fences, you still have to
> wait for all the current ones to signal. Also, we have dma-access
> without fences in i915. "I hold the reservation_object" does not imply
> you can just go and nuke the backing storage.

I was not talking about taking the lock, but rather using 
reservation_object_wait_timeout_rcu().

To be more precise you actually can't take the reservation object lock 
in an mmu_invalidate_range callback and you can only trylock it in a 
shrink_slab callback.

>> This needs to be done to make sure that the backing memory is now idle, it
>> doesn't matter if the jobs where submitted by DMA-buf importers or your own
>> driver.
>>
>> The sg tables pointing to the now released memory might live a bit longer,
>> but that is unproblematic and actually intended.
> I think that's very problematic. One reason for an IOMMU is that you
> have device access isolation, and a broken device can't access memory
> it shouldn't be able to access. From that security-in-depth point of
> view it's not cool that there's some sg tables hanging around still
> that a broken GPU could use. And let's not pretend hw is perfect,
> especially GPUs :-)

I completely agree on that, but there is unfortunately no other way.

See you simply can't take a reservation object lock in an mmu or slab 
callback, you can only trylock them.

For example it would require changing all allocations done while holding 
any reservation lock to GFP_NOIO.

>> When we would try to destroy the sg tables in an mmu_invalidate_range or
>> shrink_slab callback we would run into a lockdep horror.
> So I'm no expert on this, but I think this is exactly what we're doing
> in i915. Kinda no other way to actually free the memory without
> throwing all the nice isolation aspects of an IOMMU into the wind. Can
> you please paste the lockdeps you've seen with amdgpu when trying to
> do that?

Taking a quick look at i915 I can definitely say that this is actually 
quite buggy what you guys do here.

For coherent usage you need to install some lock to prevent concurrent 
get_user_pages(), command submission and 
invalidate_range_start/invalidate_range_end from the MMU notifier.

Otherwise you can't guarantee that you are actually accessing the right 
page in the case of a fork() or mprotect().

Felix and I hammered for quite some time on amdgpu until all of this was 
handled correctly, see drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c.

I can try to gather the lockdep splat from my mail history, but it 
essentially took us multiple years to get rid of all of them.

Regards,
Christian.

> -Daniel
>
>> Regards,
>> Christian.
>>
>>> Even so, it basically means passing a fence object down to the async
>>> callbacks for them to signal when they are complete. Just to handle the
>>> non-pipelined version. :|
>>> -Chris
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>> _______________________________________________
>> Linaro-mm-sig mailing list
>> Linaro-mm-sig at lists.linaro.org
>> https://lists.linaro.org/mailman/listinfo/linaro-mm-sig
>
>