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

[Steven Price]

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.

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);
>