diff mbox series

[5/5] drm/amdgpu: implement amdgpu_gem_prime_move_notify

Message ID 20191029104049.9011-6-christian.koenig@amd.com (mailing list archive)
State New, archived
Headers show
Series [1/5] dma-buf: add dynamic DMA-buf handling v14 | expand

Commit Message

Christian König Oct. 29, 2019, 10:40 a.m. UTC
Implement the importer side of unpinned DMA-buf handling.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 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(-)

Comments

Daniel Vetter Nov. 5, 2019, 10:52 a.m. UTC | #1
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@amd.com>
> ---
>  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?
-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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Christian König Nov. 5, 2019, 1:39 p.m. UTC | #2
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@amd.com>
>> ---
>>   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.

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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter Nov. 5, 2019, 1:50 p.m. UTC | #3
On Tue, Nov 5, 2019 at 2:39 PM Christian König
<ckoenig.leichtzumerken@gmail.com> 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@amd.com>
> >> ---
> >>   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?
-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@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
Christian König Nov. 5, 2019, 3:20 p.m. UTC | #4
Am 05.11.19 um 14:50 schrieb Daniel Vetter:
> On Tue, Nov 5, 2019 at 2:39 PM Christian König
> <ckoenig.leichtzumerken@gmail.com> 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@amd.com>
>>>> ---
>>>>    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@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
Daniel Vetter Nov. 5, 2019, 3:23 p.m. UTC | #5
On Tue, Nov 5, 2019 at 4:20 PM Koenig, Christian
<Christian.Koenig@amd.com> wrote:
>
> Am 05.11.19 um 14:50 schrieb Daniel Vetter:
> > On Tue, Nov 5, 2019 at 2:39 PM Christian König
> > <ckoenig.leichtzumerken@gmail.com> 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@amd.com>
> >>>> ---
> >>>>    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.

Ok, I think that also clears up some confusion we had around
notify_move semantics, where I wanted to add more fences from within
the callback (for the pipelined gpu pagetable clearing) and you didn't
really see the point. I dumped my overall thoughts on all things open
here on the cover letter, but yeah probably best if you wire up the pt
clearing in amdgpu first. Then we can easier see what we'll need to
funnel that through dma-buf.

Cheers, Daniel

>
> 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@lists.freedesktop.org
> >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
>
diff mbox series

Patch

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);
+}
+
 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;