Am 14.09.20 um 15:29 schrieb Christian König:
> Hi Andrew,
Sorry forgot to add Daniel as well.
>
> 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.
>
>
On Mon, Sep 14, 2020 at 03:30:47PM +0200, Christian König wrote:
> Am 14.09.20 um 15:29 schrieb Christian König:
> > 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
It doesn't look obviously safe as mmap_region() has an interesting mix
of file and vma->file
Eg it calls mapping_unmap_writable() using both routes
What about security? Is it OK that some other random file, maybe in
another process, is being linked to this mmap?
> > 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.
So the pgoff is some virtualized thing?
Jason
Am 14.09.20 um 16:06 schrieb Jason Gunthorpe:
> On Mon, Sep 14, 2020 at 03:30:47PM +0200, Christian König wrote:
>> Am 14.09.20 um 15:29 schrieb Christian König:
>>> 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
> It doesn't look obviously safe as mmap_region() has an interesting mix
> of file and vma->file
>
> Eg it calls mapping_unmap_writable() using both routes
Thanks for the hint, going to take a look at that code tomorrow.
> What about security? Is it OK that some other random file, maybe in
> another process, is being linked to this mmap?
Good question, I have no idea. That's why I send out this mail.
>>> 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.
> So the pgoff is some virtualized thing?
Yes, absolutely.
Christian.
>
> Jason
On Mon, Sep 14, 2020 at 08:26:47PM +0200, Christian K?nig wrote:
> Am 14.09.20 um 16:06 schrieb Jason Gunthorpe:
> > On Mon, Sep 14, 2020 at 03:30:47PM +0200, Christian K?nig wrote:
> > > Am 14.09.20 um 15:29 schrieb Christian K?nig:
> > > > 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
> > It doesn't look obviously safe as mmap_region() has an interesting mix
> > of file and vma->file
> >
> > Eg it calls mapping_unmap_writable() using both routes
>
> Thanks for the hint, going to take a look at that code tomorrow.
>
> > What about security? Is it OK that some other random file, maybe in
> > another process, is being linked to this mmap?
>
> Good question, I have no idea. That's why I send out this mail.
>
> > > > 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.
> > So the pgoff is some virtualized thing?
>
> Yes, absolutely.
Maybe notch more context. Conceptually the buffer objects we use to manage
gpu memory are all stand-alone objects, internally refcounted and
everything. And if you export them as a dma-buf, then they are indeed
stand-alone file descriptors like any other.
But within the driver, we generally need thousands of these, and that
tends to bring fd exhaustion problems with it. That's why all the private
buffer objects which aren't shared with other process or other drivers are
handles only valid for a specific fd instance of the drm chardev (each
open gets their own namespace), and only for ioctls done on that chardev.
And for mmap we assign fake (but unique across all open fd on it) offsets
within the overall chardev. Hence all the pgoff mangling and re-mangling.
Now for unmap_mapping_range we'd like it to find all such fake offset
aliases pointing at the one underlying buffer object:
- mmap on the dma-buf fd, at offset 0
- mmap on the drm chardev where the buffer was originally allocated, at some unique offset
- mmap on the drm chardev where the buffer was imported, again at some
(likely) different unique (for that chardev) offset.
So to make unmap_mapping_range work across the entire delegation change
we'd actually need to change the vma->vma_file and pgoff twice:
- once when forwarding from the importing drm chardev to the dma-buf
- once when forwarding from the dma-buf to the exported drm chardev fake
offset, which (mostly for historical reasons) is considered the
canonical fake offset
We can't really do the delegation in userspace because:
- the importer might not have access to the exporters drm chardev, it only
gets the dma-buf. If we'd give it the underlying drm chardev it could do
stuff like issue rendering commands, breaking the access model.
- the dma-buf fd is only used to establish the sharing, once it's imported
everywhere it generally gets closed. Userspace could re-export it and
then call mmap on that, but feels a bit contrived.
- especially on SoC platforms this has already become uapi. It's not a big
problem because the drivers that really need unmap_mapping_range to work
are the big gpu drivers with discrete vram, where mappings need to be
invalidate when moving buffer objects in/out of vram.
Hence why we'd like to be able to forward aliasing mappings and adjust the
file and pgoff, while hopefully everything keeps working. I thought this
would work, but Christian noticed it doesn't really.
Cheers, Daniel
>
> Christian.
>
> >
> > Jason
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Wed, Sep 16, 2020 at 11:53:59AM +0200, Daniel Vetter wrote:
> But within the driver, we generally need thousands of these, and that
> tends to bring fd exhaustion problems with it. That's why all the private
> buffer objects which aren't shared with other process or other drivers are
> handles only valid for a specific fd instance of the drm chardev (each
> open gets their own namespace), and only for ioctls done on that chardev.
> And for mmap we assign fake (but unique across all open fd on it) offsets
> within the overall chardev. Hence all the pgoff mangling and re-mangling.
Are they still unique struct files? Just without a fdno?
> Hence why we'd like to be able to forward aliasing mappings and adjust the
> file and pgoff, while hopefully everything keeps working. I thought this
> would work, but Christian noticed it doesn't really.
It seems reasonable to me that the dma buf should be the owner of the
VMA, otherwise like you say, there is a big mess attaching the custom
vma ops and what not to the proper dma buf.
I don't see anything obviously against this in mmap_region() - why did
Chritian notice it doesn't really work?
Jason
Am 16.09.20 um 16:07 schrieb Jason Gunthorpe:
> On Wed, Sep 16, 2020 at 11:53:59AM +0200, Daniel Vetter wrote:
>
>> But within the driver, we generally need thousands of these, and that
>> tends to bring fd exhaustion problems with it. That's why all the private
>> buffer objects which aren't shared with other process or other drivers are
>> handles only valid for a specific fd instance of the drm chardev (each
>> open gets their own namespace), and only for ioctls done on that chardev.
>> And for mmap we assign fake (but unique across all open fd on it) offsets
>> within the overall chardev. Hence all the pgoff mangling and re-mangling.
> Are they still unique struct files? Just without a fdno?
Yes, exactly.
>> Hence why we'd like to be able to forward aliasing mappings and adjust the
>> file and pgoff, while hopefully everything keeps working. I thought this
>> would work, but Christian noticed it doesn't really.
> It seems reasonable to me that the dma buf should be the owner of the
> VMA, otherwise like you say, there is a big mess attaching the custom
> vma ops and what not to the proper dma buf.
>
> I don't see anything obviously against this in mmap_region() - why did
> Chritian notice it doesn't really work?
To clarify I think this might work.
I just had the same "Is that legal?", "What about security?", etc..
questions you raised as well.
It seems like a source of trouble so I thought better ask somebody more
familiar with that.
Christian.
>
> Jason
On Wed, Sep 16, 2020 at 4:14 PM Christian König
<[email protected]> wrote:
>
> Am 16.09.20 um 16:07 schrieb Jason Gunthorpe:
> > On Wed, Sep 16, 2020 at 11:53:59AM +0200, Daniel Vetter wrote:
> >
> >> But within the driver, we generally need thousands of these, and that
> >> tends to bring fd exhaustion problems with it. That's why all the private
> >> buffer objects which aren't shared with other process or other drivers are
> >> handles only valid for a specific fd instance of the drm chardev (each
> >> open gets their own namespace), and only for ioctls done on that chardev.
> >> And for mmap we assign fake (but unique across all open fd on it) offsets
> >> within the overall chardev. Hence all the pgoff mangling and re-mangling.
> > Are they still unique struct files? Just without a fdno?
>
> Yes, exactly.
Not entirely, since dma-buf happened after drm chardev, so for that
historical reason the underlying struct file is shared, since it's the
drm chardev. But since that's per-device we don't have a problem in
practice with different vm_ops, since those are also per-device. But
yeah we could fish out some entirely hidden per-object struct file if
that's required for some mm internal reasons.
-Daniel
> >> Hence why we'd like to be able to forward aliasing mappings and adjust the
> >> file and pgoff, while hopefully everything keeps working. I thought this
> >> would work, but Christian noticed it doesn't really.
> > It seems reasonable to me that the dma buf should be the owner of the
> > VMA, otherwise like you say, there is a big mess attaching the custom
> > vma ops and what not to the proper dma buf.
> >
> > I don't see anything obviously against this in mmap_region() - why did
> > Chritian notice it doesn't really work?
>
> To clarify I think this might work.
>
> I just had the same "Is that legal?", "What about security?", etc..
> questions you raised as well.
>
> It seems like a source of trouble so I thought better ask somebody more
> familiar with that.
>
> Christian.
>
> >
> > Jason
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch