linux-kernel - Re: [PATCH] drm/ttm: fix mmap refcounting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191113163300.GS23790@phenom.ffwll.local>
Date:   Wed, 13 Nov 2019 17:33:00 +0100
From:   Daniel Vetter <daniel@...ll.ch>
To:     Gerd Hoffmann <kraxel@...hat.com>
Cc:     drm@...hat.com,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Sean Paul <sean@...rly.run>, David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        "open list:DRM DRIVERS" <dri-devel@...ts.freedesktop.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm/ttm: fix mmap refcounting

On Wed, Nov 13, 2019 at 02:56:12PM +0100, Gerd Hoffmann wrote:
> When mapping ttm objects via drm_gem_ttm_mmap() helper
> drm_gem_mmap_obj() will take an object reference.  That gets
> never released due to ttm having its own reference counting.
> 
> Fix that by dropping the gem object reference once the ttm mmap
> completed (and ttm refcount got bumped).
> 
> For that to work properly the drm_gem_object_get() call in
> drm_gem_ttm_mmap() must be moved so it happens before calling
> obj->funcs->mmap(), otherwise the gem refcount would go down
> to zero.
> 
> Fixes: 231927d939f0 ("drm/ttm: add drm_gem_ttm_mmap()")

Since the offending patch is in drm-next and we're in the merge window
freeze past -rc6 please remember to apply this to drm-misc-next-fixes.
Otherwise it'll miss the merge window.

> Signed-off-by: Gerd Hoffmann <kraxel@...hat.com>

I was wondering whether we'd need a cc: stable, in case someone is really
fast and gets the vm_close in before we finish the mmap. But I think we
should be serialized by mmap_sem here enough ...

Reviewed-by: Daniel Vetter <daniel.vetter@...ll.ch>

> ---
>  drivers/gpu/drm/drm_gem.c            | 24 ++++++++++++++----------
>  drivers/gpu/drm/drm_gem_ttm_helper.c | 13 ++++++++++++-
>  2 files changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 2f2b889096b0..000fa4a1899f 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1105,21 +1105,33 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>  	if (obj_size < vma->vm_end - vma->vm_start)
>  		return -EINVAL;
>  
> +	/* Take a ref for this mapping of the object, so that the fault
> +	 * handler can dereference the mmap offset's pointer to the object.
> +	 * This reference is cleaned up by the corresponding vm_close
> +	 * (which should happen whether the vma was created by this call, or
> +	 * by a vm_open due to mremap or partial unmap or whatever).
> +	 */
> +	drm_gem_object_get(obj);
> +
>  	if (obj->funcs && obj->funcs->mmap) {
>  		/* Remove the fake offset */
>  		vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
>  
>  		ret = obj->funcs->mmap(obj, vma);
> -		if (ret)
> +		if (ret) {
> +			drm_gem_object_put_unlocked(obj);
>  			return ret;
> +		}
>  		WARN_ON(!(vma->vm_flags & VM_DONTEXPAND));
>  	} else {
>  		if (obj->funcs && obj->funcs->vm_ops)
>  			vma->vm_ops = obj->funcs->vm_ops;
>  		else if (dev->driver->gem_vm_ops)
>  			vma->vm_ops = dev->driver->gem_vm_ops;
> -		else
> +		else {
> +			drm_gem_object_put_unlocked(obj);
>  			return -EINVAL;
> +		}
>  
>  		vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
>  		vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> @@ -1128,14 +1140,6 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>  
>  	vma->vm_private_data = obj;
>  
> -	/* Take a ref for this mapping of the object, so that the fault
> -	 * handler can dereference the mmap offset's pointer to the object.
> -	 * This reference is cleaned up by the corresponding vm_close
> -	 * (which should happen whether the vma was created by this call, or
> -	 * by a vm_open due to mremap or partial unmap or whatever).
> -	 */
> -	drm_gem_object_get(obj);
> -
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_gem_mmap_obj);
> diff --git a/drivers/gpu/drm/drm_gem_ttm_helper.c b/drivers/gpu/drm/drm_gem_ttm_helper.c
> index 7412bfc5c05a..605a8a3da7f9 100644
> --- a/drivers/gpu/drm/drm_gem_ttm_helper.c
> +++ b/drivers/gpu/drm/drm_gem_ttm_helper.c
> @@ -64,8 +64,19 @@ int drm_gem_ttm_mmap(struct drm_gem_object *gem,
>  		     struct vm_area_struct *vma)
>  {
>  	struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
> +	int ret;
>  
> -	return ttm_bo_mmap_obj(vma, bo);
> +	ret = ttm_bo_mmap_obj(vma, bo);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * ttm has its own object refcounting, so drop gem reference
> +	 * to avoid double accounting counting.
> +	 */
> +	drm_gem_object_put_unlocked(gem);
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL(drm_gem_ttm_mmap);
>  
> -- 
> 2.18.1
> 

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