On Mon, Feb 22, 2021 at 03:24:17PM +0100, Thomas Zimmermann wrote:
> Hi
>
> Am 17.02.21 um 17:59 schrieb Neil Roberts:
> > When mmapping the shmem, it would previously adjust the pgoff in the
> > vm_area_struct to remove the fake offset that is added to be able to
> > identify the buffer. This patch removes the adjustment and makes the
> > fault handler use the vm_fault address to calculate the page offset
> > instead. Although using this address is apparently discouraged, several
> > DRM drivers seem to be doing it anyway.
> >
> > The problem with removing the pgoff is that it prevents
> > drm_vma_node_unmap from working because that searches the mapping tree
> > by address. That doesn't work because all of the mappings are at offset
> > 0. drm_vma_node_unmap is being used by the shmem helpers when purging
> > the buffer.
>
> I just want to ask if this is how the mmap callbacks are supposed to work
> now?
>
> I have a number of patches in here that convert several drivers to the GEM
> object's mmap callback. They might not be affected by the vm_pgoff bug, but
> I'd like to submit something that could work in the longer term.
Yeah we pretty much require the uniq vm_pgoff for runtime unmapping.
Especially with more dynamic memory managers like ttm that move buffers
around - for more static ones (most of the armsoc ones) it's just a bit a
security issue since you can potentially access memory after it's gone.
-Daniel
> Best regards
> Thomas
>
> >
> > It looks like panfrost is using drm_gem_shmem_purge so this might fix a
> > potential bug there.
> >
> > Signed-off-by: Neil Roberts <nroberts@xxxxxxxxxx>
> > ---
> > drivers/gpu/drm/drm_gem_shmem_helper.c | 12 +++++++-----
> > 1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > index 9825c378dfa6..4b14157f1962 100644
> > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > @@ -526,11 +526,16 @@ static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf)
> > struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
> > loff_t num_pages = obj->size >> PAGE_SHIFT;
> > struct page *page;
> > + pgoff_t page_offset;
> > - if (vmf->pgoff >= num_pages || WARN_ON_ONCE(!shmem->pages))
> > + /* We don't use vmf->pgoff since that has the fake offset */
> > + page_offset = (vmf->address - vma->vm_start) >> PAGE_SHIFT;
> > +
> > + if (page_offset < 0 || page_offset >= num_pages ||
> > + WARN_ON_ONCE(!shmem->pages))
> > return VM_FAULT_SIGBUS;
> > - page = shmem->pages[vmf->pgoff];
> > + page = shmem->pages[page_offset];
> > return vmf_insert_page(vma, vmf->address, page);
> > }
> > @@ -581,9 +586,6 @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
> > struct drm_gem_shmem_object *shmem;
> > int ret;
> > - /* Remove the fake offset */
> > - vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
> > -
> > if (obj->import_attach) {
> > /* Drop the reference drm_gem_mmap_obj() acquired.*/
> > drm_gem_object_put(obj);
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
