Am 16.09.20 um 11:53 schrieb Daniel Vetter: > 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. Well to be clear I'm still not sure if that works or not :) 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. Regards, Christian. > > Cheers, Daniel > > >> Christian. >> >>> Jason