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

[Daniel Vetter]

On Thu, Feb 18, 2021 at 4:26 PM Steven Price <steven.price at arm.com> wrote:
>
> On 17/02/2021 16:59, Neil Roberts wrote:
> > 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.
> >
> > It looks like panfrost is using drm_gem_shmem_purge so this might fix a
> > potential bug there.
> >
> > Signed-off-by: Neil Roberts <nroberts at igalia.com>
>
> As the test robot points out pgoff_t is unsigned, so the <0 test makes
> no sense. But apart from that it looks good to me.
>
> I think this is worth a "Fixes:" line too - as you point out
> drm_vma_node_unmap() won't be working correctly - which means we're
> potentially leaving user space with pages pointing to freed memory - not
> good! 17acb9f35ed7 is my best guess at the commit that introduced this.

Yeah plus Cc: stable for backporting and I think an igt or similar for
panfrost to check this works correctly would be pretty good too. Since
if it took us over 1 year to notice this bug it's pretty clear that
normal testing doesn't catch this. So very likely we'll break this
again.

btw for testing shrinkers recommended way is to have a debugfs file
that just force-shrinks everything. That way you avoid all the trouble
that tend to happen when you drive a system close to OOM on linux, and
it's also much faster.
-Daniel

> Steve
>
> > ---
> >   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);
> >
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch