Re: [PATCH] drm/shmem-helper: Don't remove the offset in vm_area_struct pgoff

[Daniel Vetter <daniel@ffwll.ch>]

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@igalia.com>
> > ---
> >   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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel