On Thu, Feb 18, 2021 at 4:26 PM Steven Price <steven.price@xxxxxxx> 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@xxxxxxxxxx>
>
> 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@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
