Re: [PATCH v4 4/9] drm/rockchip/rockchip_drm_gem.c: Convert to use vm_insert_range

[Russell King - ARM Linux]

On Tue, Dec 18, 2018 at 01:53:34AM +0530, Souptick Joarder wrote:
> Convert to use vm_insert_range() to map range of kernel
> memory to user vma.
> 
> Signed-off-by: Souptick Joarder <jrdr.linux@xxxxxxxxx>
> Tested-by: Heiko Stuebner <heiko@xxxxxxxxx>
> Acked-by: Heiko Stuebner <heiko@xxxxxxxxx>
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 19 ++-----------------
>  1 file changed, 2 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> index a8db758..8279084 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> @@ -221,26 +221,11 @@ static int rockchip_drm_gem_object_mmap_iommu(struct drm_gem_object *obj,
>  					      struct vm_area_struct *vma)
>  {
>  	struct rockchip_gem_object *rk_obj = to_rockchip_obj(obj);
> -	unsigned int i, count = obj->size >> PAGE_SHIFT;
>  	unsigned long user_count = vma_pages(vma);
> -	unsigned long uaddr = vma->vm_start;
>  	unsigned long offset = vma->vm_pgoff;
> -	unsigned long end = user_count + offset;
> -	int ret;
> -
> -	if (user_count == 0)
> -		return -ENXIO;
> -	if (end > count)
> -		return -ENXIO;
>  
> -	for (i = offset; i < end; i++) {
> -		ret = vm_insert_page(vma, uaddr, rk_obj->pages[i]);
> -		if (ret)
> -			return ret;
> -		uaddr += PAGE_SIZE;
> -	}
> -
> -	return 0;
> +	return vm_insert_range(vma, vma->vm_start, rk_obj->pages + offset,
> +				user_count - offset);

This looks like a change in behaviour.

If user_count is zero, and offset is zero, then we pass into
vm_insert_range() a page_count of zero, and vm_insert_range() does
nothing and returns zero.

However, as we can see from the above code, the original behaviour
was to return -ENXIO in that case.

The other thing that I'm wondering is that if (eg) count is 8 (the
object is 8 pages), offset is 2, and the user requests mapping 6
pages (user_count = 6), then we call vm_insert_range() with a
pages of rk_obj->pages + 2, and a pages_count of 6 - 2 = 4. So we
end up inserting four pages.

The original code would calculate end = 6 + 2 = 8.  i would iterate
from 2 through 8, inserting six pages.

(I hadn't spotted that second issue until I'd gone through the
calculations manually - which is worrying.)

I don't have patches 5 through 9 to look at, but I'm concerned that
similar issues also exist in those patches.

I'm concerned that this series seems to be introducing subtle bugs,
it seems to be unnecessarily difficult to use this function correctly.
I think your existing proposal for vm_insert_range() provides an
interface that is way too easy to get wrong, and, therefore, is the
wrong interface.

I think it would be way better to have vm_insert_range() take the
object page array without any offset adjustment, and the object
page_count again without any adjustment, and have vm_insert_range()
itself handle the offsetting and VMA size validation.  That would
then give a simple interface and probably give a further reduction
in code at each call site.

Thanks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up