Hi Andrew,
I'm the new DMA-buf maintainer and Daniel and others came up with patches extending the use of the dma_buf_mmap() function.
Now this function is doing something a bit odd by changing the vma->vm_file while installing a VMA in the mmap() system call
The background here is that DMA-buf allows device drivers to export buffer which are then imported into another device driver. The mmap() handler of the importing device driver then find that the pgoff belongs to the exporting device and so redirects the mmap() call there.
In other words user space calls mmap() on one file descriptor, but get a different one mapped into your virtual address space.
My question is now: Is that legal or can you think of something which breaks here?
If it's not legal we should probably block any new users of the dma_buf_mmap() function and consider what should happen with the two existing ones.
If that is legal I would like to document this by adding a new vma_set_file() function which does the necessary reference count dance.
Thanks in advance,
Christian.
This reverts commit 26d3ac3cb04d171a861952e89324e347598a347f.
We need to figure out if dma_buf_mmap() is valid or not first.
Signed-off-by: Christian König <[email protected]>
---
drivers/gpu/drm/drm_gem_shmem_helper.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 0a952f27c184..cd727343f72b 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -594,9 +594,6 @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
/* Remove the fake offset */
vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
- if (obj->import_attach)
- return dma_buf_mmap(obj->dma_buf, vma, 0);
-
shmem = to_drm_gem_shmem_obj(obj);
ret = drm_gem_shmem_get_pages(shmem);
--
2.17.1
On Mon, Sep 14, 2020 at 3:29 PM Christian König
<[email protected]> wrote:
>
> This reverts commit 26d3ac3cb04d171a861952e89324e347598a347f.
>
> We need to figure out if dma_buf_mmap() is valid or not first.
>
> Signed-off-by: Christian König <[email protected]>
The trouble is that doing dma-buf mmap by looking at the struct pages
behind the sg list and just inserting those into userspace doesn't
really work any better. You still won't get the unmap_mapping_range
and hence pte shoot-down. So maybe dma_buf_mmap forwarding doesn't
work, but this doesn't make it any better.
Also commit message should probably explain a bit the context here,
not a lot of people have been in our private discussion on this.
-Daniel
> ---
> drivers/gpu/drm/drm_gem_shmem_helper.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 0a952f27c184..cd727343f72b 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -594,9 +594,6 @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
> /* Remove the fake offset */
> vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
>
> - if (obj->import_attach)
> - return dma_buf_mmap(obj->dma_buf, vma, 0);
> -
> shmem = to_drm_gem_shmem_obj(obj);
>
> ret = drm_gem_shmem_get_pages(shmem);
> --
> 2.17.1
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Am 15.09.20 um 12:39 schrieb Daniel Vetter:
> On Mon, Sep 14, 2020 at 3:29 PM Christian König
> <[email protected]> wrote:
>> This reverts commit 26d3ac3cb04d171a861952e89324e347598a347f.
>>
>> We need to figure out if dma_buf_mmap() is valid or not first.
>>
>> Signed-off-by: Christian König <[email protected]>
> The trouble is that doing dma-buf mmap by looking at the struct pages
> behind the sg list and just inserting those into userspace doesn't
> really work any better. You still won't get the unmap_mapping_range
> and hence pte shoot-down. So maybe dma_buf_mmap forwarding doesn't
> work, but this doesn't make it any better.
Good point. Question is what should we do? Return -EPERM?
>
> Also commit message should probably explain a bit the context here,
> not a lot of people have been in our private discussion on this.
Well, that's certain true.
Christian.
> -Daniel
>
>> ---
>> drivers/gpu/drm/drm_gem_shmem_helper.c | 3 ---
>> 1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> index 0a952f27c184..cd727343f72b 100644
>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> @@ -594,9 +594,6 @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
>> /* Remove the fake offset */
>> vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
>>
>> - if (obj->import_attach)
>> - return dma_buf_mmap(obj->dma_buf, vma, 0);
>> -
>> shmem = to_drm_gem_shmem_obj(obj);
>>
>> ret = drm_gem_shmem_get_pages(shmem);
>> --
>> 2.17.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> [email protected]
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
On Tue, Sep 15, 2020 at 1:03 PM Christian König
<[email protected]> wrote:
>
> Am 15.09.20 um 12:39 schrieb Daniel Vetter:
> > On Mon, Sep 14, 2020 at 3:29 PM Christian König
> > <[email protected]> wrote:
> >> This reverts commit 26d3ac3cb04d171a861952e89324e347598a347f.
> >>
> >> We need to figure out if dma_buf_mmap() is valid or not first.
> >>
> >> Signed-off-by: Christian König <[email protected]>
> > The trouble is that doing dma-buf mmap by looking at the struct pages
> > behind the sg list and just inserting those into userspace doesn't
> > really work any better. You still won't get the unmap_mapping_range
> > and hence pte shoot-down. So maybe dma_buf_mmap forwarding doesn't
> > work, but this doesn't make it any better.
>
> Good point. Question is what should we do? Return -EPERM?
IIrc there's userspace which expects this to work, but I think it's
also only trying to do this with simpler drivers that don't need
unmap_mapping_range to work correctly. Or it's simply that no one ever
reported the bugs. It's a bit a mess :-/
-Daniel
>
> >
> > Also commit message should probably explain a bit the context here,
> > not a lot of people have been in our private discussion on this.
>
> Well, that's certain true.
>
> Christian.
>
> > -Daniel
> >
> >> ---
> >> drivers/gpu/drm/drm_gem_shmem_helper.c | 3 ---
> >> 1 file changed, 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> >> index 0a952f27c184..cd727343f72b 100644
> >> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> >> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> >> @@ -594,9 +594,6 @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
> >> /* Remove the fake offset */
> >> vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
> >>
> >> - if (obj->import_attach)
> >> - return dma_buf_mmap(obj->dma_buf, vma, 0);
> >> -
> >> shmem = to_drm_gem_shmem_obj(obj);
> >>
> >> ret = drm_gem_shmem_get_pages(shmem);
> >> --
> >> 2.17.1
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> [email protected]
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Wed, Sep 16, 2020 at 1:45 PM Christian König
<[email protected]> wrote:
>
> [SNIP]
>
> But Jason pointed me to the right piece of code. See this comment in in mmap_region():
>
> /* ->mmap() can change vma->vm_file, but must guarantee that
> * vma_link() below can deny write-access if VM_DENYWRITE is set
> * and map writably if VM_SHARED is set. This usually means the
> * new file must not have been exposed to user-space, yet.
> */
> vma->vm_file = get_file(file);
> error = call_mmap(file, vma);
>
>
> So changing vma->vm_file is allowed at least under certain circumstances.
>
> Only the "file must not have been exposed to user-space, yet" part still needs double checking. Currently working on that.
>
>
> Ok, I think we can guarantee for all DMA-bufs what is required here.
>
> While searching the code I've found that at least vgem_prime_mmap() and i915_gem_dmabuf_mmap() are doing the same thing of modifying vma->vm_file.
>
> So I'm leaning towards that this works as expected and we should just document this properly.
>
> Daniel and Jason what do you think?
Well I can claim I started this, so I started out with naively
assuming that it Just Works :-)
I think we already have vgem/i915 prime testcases in igt which try to
excercise this mmap forwarding, including provoking pte shoot-downs.
So I think best would be if you could also add a variant for amdgpu,
to make sure this really works and keeps working.
Plus ofc the documentation patch so that core mm folks can officially
ack this as legit.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch