On Tue, Dec 18, 2018 at 3:27 PM Russell King - ARM Linux
<linux@xxxxxxxxxxxxxxx> wrote:
>
> 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.
I think these checks are not necessary. I am not sure if we get into mmap
handlers of driver with user_count = 0.
>
> 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.
Considering the scenario, user_count will remain 8 (user_count =
vma_pages(vma) ). ? No ?
Then we call vm_insert_range() with a pages of rk_obj->pages + 2, and
a pages_count
of 8 - 2 = 6. So we end up inserting 6 pages.
Please correct me if I am wrong.
>
> 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.
yes, you were not cc'd for 5 - 9. Below are the patchwork links -
https://patchwork.kernel.org/patch/10734269/
https://patchwork.kernel.org/patch/10734271/
https://patchwork.kernel.org/patch/10734273/
https://patchwork.kernel.org/patch/10734277/
https://patchwork.kernel.org/patch/10734279/
>
> 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
