| 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 |
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
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
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 >
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 >
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 --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;
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(-)