On Thu, Oct 4, 2018 at 1:28 AM Miguel Ojeda
<miguel.ojeda.sandonis@xxxxxxxxx> wrote:
>
> Hi Souptick,
>
> On Wed, Oct 3, 2018 at 8:55 PM Souptick Joarder <jrdr.linux@xxxxxxxxx> wrote:
> >
> > vm_insert_kmem_page is similar to vm_insert_page and will
> > be used by drivers to map kernel (kmalloc/vmalloc/pages)
> > allocated memory to user vma.
> >
> > Going forward, the plan is to restrict future drivers not
> > to use vm_insert_page ( *it will generate new errno to
> > VM_FAULT_CODE mapping code for new drivers which were already
> > cleaned up for existing drivers*) in #PF (page fault handler)
> > context but to make use of vmf_insert_page which returns
> > VMF_FAULT_CODE and that is not possible until both vm_insert_page
> > and vmf_insert_page API exists.
> >
> > But there are some consumers of vm_insert_page which use it
> > outside #PF context. straight forward conversion of vm_insert_page
> > to vmf_insert_page won't work there as those function calls expects
> > errno not vm_fault_t in return.
> >
> > These are the approaches which could have been taken to handle
> > this scenario -
> >
> > * Replace vm_insert_page with vmf_insert_page and then write few
> > extra lines of code to convert VM_FAULT_CODE to errno which
> > makes driver users more complex ( also the reverse mapping errno to
> > VM_FAULT_CODE have been cleaned up as part of vm_fault_t migration ,
> > not preferred to introduce anything similar again)
> >
> > * Maintain both vm_insert_page and vmf_insert_page and use it in
> > respective places. But it won't gurantee that vm_insert_page will
> > never be used in #PF context.
> >
> > * Introduce a similar API like vm_insert_page, convert all non #PF
> > consumer to use it and finally remove vm_insert_page by converting
> > it to vmf_insert_page.
> >
> > And the 3rd approach was taken by introducing vm_insert_kmem_page().
>
> This looks better than the previous one of adding non-trivial code to
> each driver, thank you!
>
> A couple of comments below.
>
> >
> > In short, vmf_insert_page will be used in page fault handlers
> > context and vm_insert_kmem_page will be used to map kernel
> > memory to user vma outside page fault handlers context.
> >
> > Few drivers are converted to use vm_insert_kmem_page(). This will
> > allow both to review the api and that it serves it purpose. other
> > consumers of vm_insert_page (*used in non #PF context*) will be
> > replaced by vm_insert_kmem_page, but in separate patches.
> >
>
> other -> Other
>
> Also, as far as I can see, there are only a few vm_insert_page users
> remaining. With the new function, they should be trivial to convert,
> no? Therefore, could we do them all in one go, possibly in a patch
> series?
yes, all the user can be converted in a patch series.
>
> Or, maybe, even better: wait until you remove the vm_* functions and
> simply reuse vm_insert_page for this -- that way you don't need a new
> name and you don't have to change any of the last users (I mean the
> drivers using it outside the page fault handlers).
>
> > Signed-off-by: Souptick Joarder <jrdr.linux@xxxxxxxxx>
> > ---
> > v2: Few non #PF consumers of vm_insert_page are converted
> > to use vm_insert_kmem_page in patch v2.
> >
> > Updated the change log.
> >
> > arch/arm/mm/dma-mapping.c | 2 +-
> > drivers/auxdisplay/cfag12864bfb.c | 2 +-
> > drivers/auxdisplay/ht16k33.c | 2 +-
> > drivers/firewire/core-iso.c | 2 +-
> > drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 2 +-
> > include/linux/mm.h | 2 +
> > kernel/kcov.c | 4 +-
> > mm/memory.c | 69 +++++++++++++++++++++++++++++
> > mm/nommu.c | 7 +++
> > mm/vmalloc.c | 2 +-
> > 10 files changed, 86 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> > index 6656647..58d7971 100644
> > --- a/arch/arm/mm/dma-mapping.c
> > +++ b/arch/arm/mm/dma-mapping.c
> > @@ -1598,7 +1598,7 @@ static int __arm_iommu_mmap_attrs(struct device *dev, struct vm_area_struct *vma
> > pages += off;
> >
> > do {
> > - int ret = vm_insert_page(vma, uaddr, *pages++);
> > + int ret = vm_insert_kmem_page(vma, uaddr, *pages++);
> > if (ret) {
> > pr_err("Remapping memory failed: %d\n", ret);
> > return ret;
> > diff --git a/drivers/auxdisplay/cfag12864bfb.c b/drivers/auxdisplay/cfag12864bfb.c
> > index 40c8a55..82fd627 100644
> > --- a/drivers/auxdisplay/cfag12864bfb.c
> > +++ b/drivers/auxdisplay/cfag12864bfb.c
> > @@ -52,7 +52,7 @@
> >
> > static int cfag12864bfb_mmap(struct fb_info *info, struct vm_area_struct *vma)
> > {
> > - return vm_insert_page(vma, vma->vm_start,
> > + return vm_insert_kmem_page(vma, vma->vm_start,
> > virt_to_page(cfag12864b_buffer));
> > }
> >
> > diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
> > index a43276c..64de30b 100644
> > --- a/drivers/auxdisplay/ht16k33.c
> > +++ b/drivers/auxdisplay/ht16k33.c
> > @@ -224,7 +224,7 @@ static int ht16k33_mmap(struct fb_info *info, struct vm_area_struct *vma)
> > {
> > struct ht16k33_priv *priv = info->par;
> >
> > - return vm_insert_page(vma, vma->vm_start,
> > + return vm_insert_kmem_page(vma, vma->vm_start,
> > virt_to_page(priv->fbdev.buffer));
> > }
> >
> > diff --git a/drivers/firewire/core-iso.c b/drivers/firewire/core-iso.c
> > index 051327a..5f1548d 100644
> > --- a/drivers/firewire/core-iso.c
> > +++ b/drivers/firewire/core-iso.c
> > @@ -112,7 +112,7 @@ int fw_iso_buffer_map_vma(struct fw_iso_buffer *buffer,
> >
> > uaddr = vma->vm_start;
> > for (i = 0; i < buffer->page_count; i++) {
> > - err = vm_insert_page(vma, uaddr, buffer->pages[i]);
> > + err = vm_insert_kmem_page(vma, uaddr, buffer->pages[i]);
> > if (err)
> > return err;
> >
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> > index a8db758..57eb7af 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> > @@ -234,7 +234,7 @@ static int rockchip_drm_gem_object_mmap_iommu(struct drm_gem_object *obj,
> > return -ENXIO;
> >
> > for (i = offset; i < end; i++) {
> > - ret = vm_insert_page(vma, uaddr, rk_obj->pages[i]);
> > + ret = vm_insert_kmem_page(vma, uaddr, rk_obj->pages[i]);
> > if (ret)
> > return ret;
> > uaddr += PAGE_SIZE;
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index a61ebe8..5f42d35 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -2477,6 +2477,8 @@ unsigned long change_prot_numa(struct vm_area_struct *vma,
> > struct vm_area_struct *find_extend_vma(struct mm_struct *, unsigned long addr);
> > int remap_pfn_range(struct vm_area_struct *, unsigned long addr,
> > unsigned long pfn, unsigned long size, pgprot_t);
> > +int vm_insert_kmem_page(struct vm_area_struct *vma, unsigned long addr,
> > + struct page *page);
> > int vm_insert_page(struct vm_area_struct *, unsigned long addr, struct page *);
> > int vm_insert_pfn(struct vm_area_struct *vma, unsigned long addr,
> > unsigned long pfn);
> > diff --git a/kernel/kcov.c b/kernel/kcov.c
> > index 3ebd09e..2afaeb4 100644
> > --- a/kernel/kcov.c
> > +++ b/kernel/kcov.c
> > @@ -293,8 +293,8 @@ static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
> > spin_unlock(&kcov->lock);
> > for (off = 0; off < size; off += PAGE_SIZE) {
> > page = vmalloc_to_page(kcov->area + off);
> > - if (vm_insert_page(vma, vma->vm_start + off, page))
> > - WARN_ONCE(1, "vm_insert_page() failed");
> > + if (vm_insert_kmem_page(vma, vma->vm_start + off, page))
> > + WARN_ONCE(1, "vm_insert_kmem_page() failed");
> > }
> > return 0;
> > }
> > diff --git a/mm/memory.c b/mm/memory.c
> > index c467102..b800c10 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -1682,6 +1682,75 @@ pte_t *__get_locked_pte(struct mm_struct *mm, unsigned long addr,
> > return pte_alloc_map_lock(mm, pmd, addr, ptl);
> > }
> >
> > +static int insert_kmem_page(struct vm_area_struct *vma, unsigned long addr,
> > + struct page *page, pgprot_t prot)
> > +{
> > + struct mm_struct *mm = vma->vm_mm;
> > + int retval;
> > + pte_t *pte;
> > + spinlock_t *ptl;
> > +
> > + retval = -EINVAL;
> > + if (PageAnon(page))
> > + goto out;
> > + retval = -ENOMEM;
> > + flush_dcache_page(page);
> > + pte = get_locked_pte(mm, addr, &ptl);
> > + if (!pte)
> > + goto out;
> > + retval = -EBUSY;
> > + if (!pte_none(*pte))
> > + goto out_unlock;
> > +
> > + get_page(page);
> > + inc_mm_counter_fast(mm, mm_counter_file(page));
> > + page_add_file_rmap(page, false);
> > + set_pte_at(mm, addr, pte, mk_pte(page, prot));
> > +
> > + retval = 0;
> > + pte_unmap_unlock(pte, ptl);
> > + return retval;
> > +out_unlock:
> > + pte_unmap_unlock(pte, ptl);
> > +out:
> > + return retval;
> > +}
> > +
> > +/**
> > + * vm_insert_kmem_page - insert single page into user vma
> > + * @vma: user vma to map to
> > + * @addr: target user address of this page
> > + * @page: source kernel page
> > + *
> > + * This allows drivers to insert individual kernel memory into a user vma.
> > + * This API should be used outside page fault handlers context.
> > + *
> > + * Previously the same has been done with vm_insert_page by drivers. But
> > + * vm_insert_page will be converted to vmf_insert_page and will be used
> > + * in fault handlers context and return type of vmf_insert_page will be
> > + * vm_fault_t type.
>
> This is a "temporal" comment, i.e. it refers to things that are
> happening at the moment -- I would say that should be part of the
> commit message, not the code, since it will be obsolete soon. Also,
> consider that, in a way, vm_insert_page is actually being replaced by
> vmf_insert_page only in one of the use cases (the other being replaced
> by this). Maybe you could instead say something like:
>
> In the past, vm_insert_page was used for this purpose. Do not use
> vmf_insert_page because...
>
> and leave the full explanation in the commit.
Sure , I will work on it.
>
> > + *
> > + * But there are places where drivers need to map kernel memory into user
> > + * vma outside fault handlers context. As vmf_insert_page will be restricted
> > + * to use within page fault handlers, vm_insert_kmem_page could be used
> > + * to map kernel memory to user vma outside fault handlers context.
> > + */
>
> Ditto.
>
> > +int vm_insert_kmem_page(struct vm_area_struct *vma, unsigned long addr,
> > + struct page *page)
> > +{
> > + if (addr < vma->vm_start || addr >= vma->vm_end)
> > + return -EFAULT;
> > + if (!page_count(page))
> > + return -EINVAL;
> > + if (!(vma->vm_flags & VM_MIXEDMAP)) {
> > + BUG_ON(down_read_trylock(&vma->vm_mm->mmap_sem));
> > + BUG_ON(vma->vm_flags & VM_PFNMAP);
> > + vma->vm_flags |= VM_MIXEDMAP;
> > + }
> > + return insert_kmem_page(vma, addr, page, vma->vm_page_prot);
> > +}
> > +EXPORT_SYMBOL(vm_insert_kmem_page);
> > +
> > /*
> > * This is the old fallback for page remapping.
> > *
> > diff --git a/mm/nommu.c b/mm/nommu.c
> > index e4aac33..153b8c8 100644
> > --- a/mm/nommu.c
> > +++ b/mm/nommu.c
> > @@ -473,6 +473,13 @@ int vm_insert_page(struct vm_area_struct *vma, unsigned long addr,
> > }
> > EXPORT_SYMBOL(vm_insert_page);
> >
> > +int vm_insert_kmem_page(struct vm_area_struct *vma, unsigned long addr,
> > + struct page *page)
> > +{
> > + return -EINVAL;
> > +}
> > +EXPORT_SYMBOL(vm_insert_kmem_page);
> > +
> > /*
> > * sys_brk() for the most part doesn't need the global kernel
> > * lock, except when an application is doing something nasty
> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > index a728fc4..61d279f 100644
> > --- a/mm/vmalloc.c
> > +++ b/mm/vmalloc.c
> > @@ -2251,7 +2251,7 @@ int remap_vmalloc_range_partial(struct vm_area_struct *vma, unsigned long uaddr,
> > struct page *page = vmalloc_to_page(kaddr);
> > int ret;
> >
> > - ret = vm_insert_page(vma, uaddr, page);
> > + ret = vm_insert_kmem_page(vma, uaddr, page);
> > if (ret)
> > return ret;
> >
> > --
> > 1.9.1
> >
>
> Cheers,
> Miguel
