Am 05.11.19 um 14:50 schrieb Daniel Vetter:
> On Tue, Nov 5, 2019 at 2:39 PM Christian König
> <ckoenig.leichtzumerken@xxxxxxxxx> wrote:
>> Am 05.11.19 um 11:52 schrieb Daniel Vetter:
>>> On Tue, Oct 29, 2019 at 11:40:49AM +0100, Christian König wrote:
>>>> Implement the importer side of unpinned DMA-buf handling.
>>>>
>>>> Signed-off-by: Christian König <christian.koenig@xxxxxxx>
>>>> ---
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 28 ++++++++++++++++++++-
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 6 +++++
>>>> 2 files changed, 33 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>>>> index 3629cfe53aad..af39553c51ad 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>>>> @@ -456,7 +456,33 @@ amdgpu_dma_buf_create_obj(struct drm_device *dev, struct dma_buf *dma_buf)
>>>> return ERR_PTR(ret);
>>>> }
>>>>
>>>> +/**
>>>> + * amdgpu_dma_buf_move_notify - &attach.move_notify implementation
>>>> + *
>>>> + * @attach: the DMA-buf attachment
>>>> + *
>>>> + * Invalidate the DMA-buf attachment, making sure that the we re-create the
>>>> + * mapping before the next use.
>>>> + */
>>>> +static void
>>>> +amdgpu_dma_buf_move_notify(struct dma_buf_attachment *attach)
>>>> +{
>>>> + struct ttm_operation_ctx ctx = { false, false };
>>>> + struct drm_gem_object *obj = attach->importer_priv;
>>>> + struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
>>>> + struct ttm_placement placement = {};
>>>> + int r;
>>>> +
>>>> + if (bo->tbo.mem.mem_type == TTM_PL_SYSTEM)
>>>> + return;
>>>> +
>>>> + r = ttm_bo_validate(&bo->tbo, &placement, &ctx);
>>>> + if (r)
>>>> + DRM_ERROR("Failed to invalidate DMA-buf import (%d))\n", r);
>>> Where do you update pagetables?
>>>
>>> The only thing I've found is in the amdgpu CS code, which is way to late
>>> for this stuff here. Plus TTM doesn't handle virtual memory at all (aside
>>> from the gart tt), so clearly you need to call into amdgpu code somewhere
>>> for this. But I didn't find it, neither in your ->move_notify nor the
>>> ->move callback in ttm_bo_driver.
>>>
>>> How does this work?
>> Page tables are not updated until the next command submission, e.g. in
>> amdgpu_cs.c
>>
>> This is save since all previous command submissions are added to the
>> dma_resv object as fences and the dma_buf can't be moved before those
>> are signaled.
> Hm, I thought you still allow explicit buffer lists for each cs in
> amdgpu? Code looks at least like that, not everything goes through the
> context working set stuff.
>
> How do you prevent the security leak if userspace simply lies about
> not using a given buffer in a batch, and then abusing that to read
> that virtual address range anyway and peek at whatever is now going to
> be there when an eviction happened?
Oh, yeah that is a really good point. And no that isn't handled
correctly at all.
I wanted to rework that for quite some time now, but always got into
issues with TTM.
Thanks for the notice, so I need to put my TTM rework before of this.
Crap, that adds a whole bunch of TODOs to my list.
Regards,
Christian.
> -Daniel
>
>> Christian.
>>
>>> -Daniel
>>>
>>>> +}
>>>> +
>>>> static const struct dma_buf_attach_ops amdgpu_dma_buf_attach_ops = {
>>>> + .move_notify = amdgpu_dma_buf_move_notify
>>>> };
>>>>
>>>> /**
>>>> @@ -492,7 +518,7 @@ struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev,
>>>> return obj;
>>>>
>>>> attach = dma_buf_dynamic_attach(dma_buf, dev->dev,
>>>> - &amdgpu_dma_buf_attach_ops, NULL);
>>>> + &amdgpu_dma_buf_attach_ops, obj);
>>>> if (IS_ERR(attach)) {
>>>> drm_gem_object_put(obj);
>>>> return ERR_CAST(attach);
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> index ac776d2620eb..cfa46341c9a7 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> @@ -861,6 +861,9 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
>>>> return 0;
>>>> }
>>>>
>>>> + if (bo->tbo.base.import_attach)
>>>> + dma_buf_pin(bo->tbo.base.import_attach);
>>>> +
>>>> bo->flags |= AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS;
>>>> /* force to pin into visible video ram */
>>>> if (!(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS))
>>>> @@ -944,6 +947,9 @@ int amdgpu_bo_unpin(struct amdgpu_bo *bo)
>>>>
>>>> amdgpu_bo_subtract_pin_size(bo);
>>>>
>>>> + if (bo->tbo.base.import_attach)
>>>> + dma_buf_unpin(bo->tbo.base.import_attach);
>>>> +
>>>> for (i = 0; i < bo->placement.num_placement; i++) {
>>>> bo->placements[i].lpfn = 0;
>>>> bo->placements[i].flags &= ~TTM_PL_FLAG_NO_EVICT;
>>>> --
>>>> 2.17.1
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@xxxxxxxxxxxxxxxxxxxxx
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
