On Wed, Oct 7, 2020 at 10:33 PM John Hubbard jhubbard@nvidia.com wrote:

On 10/7/20 9:44 AM, Daniel Vetter wrote:

...

All we need are a pages array, pin_user_pages_fast can give us that directly. Plus this avoids the entire raw pfn side of get_vaddr_frames.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Jason Gunthorpe jgg@ziepe.ca Cc: Inki Dae inki.dae@samsung.com Cc: Joonyoung Shim jy0922.shim@samsung.com Cc: Seung-Woo Kim sw0312.kim@samsung.com Cc: Kyungmin Park kyungmin.park@samsung.com Cc: Kukjin Kim kgene@kernel.org Cc: Krzysztof Kozlowski krzk@kernel.org Cc: Andrew Morton akpm@linux-foundation.org Cc: John Hubbard jhubbard@nvidia.com Cc: Jérôme Glisse jglisse@redhat.com Cc: Jan Kara jack@suse.cz Cc: Dan Williams dan.j.williams@intel.com Cc: linux-mm@kvack.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-samsung-soc@vger.kernel.org Cc: linux-media@vger.kernel.org

---

drivers/gpu/drm/exynos/Kconfig | 1 - drivers/gpu/drm/exynos/exynos_drm_g2d.c | 48 ++++++++++++------------- 2 files changed, 22 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/exynos/Kconfig b/drivers/gpu/drm/exynos/Kconfig index 6417f374b923..43257ef3c09d 100644 --- a/drivers/gpu/drm/exynos/Kconfig +++ b/drivers/gpu/drm/exynos/Kconfig @@ -88,7 +88,6 @@ comment "Sub-drivers" config DRM_EXYNOS_G2D bool "G2D" depends on VIDEO_SAMSUNG_S5P_G2D=n || COMPILE_TEST

• select FRAME_VECTOR
  help
    Choose this option if you want to use Exynos G2D for DRM.
  
  

diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c b/drivers/gpu/drm/exynos/exynos_drm_g2d.c index 967a5cdc120e..c83f6faac9de 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c +++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c @@ -205,7 +205,8 @@ struct g2d_cmdlist_userptr { dma_addr_t dma_addr; unsigned long userptr; unsigned long size;

• struct frame_vector     *vec;
  

• struct page             **pages;
  

• unsigned int            npages;
  struct sg_table         *sgt;
  atomic_t                refcount;
  bool                    in_pool;
  

@@ -378,7 +379,7 @@ static void g2d_userptr_put_dma_addr(struct g2d_data *g2d, bool force) { struct g2d_cmdlist_userptr *g2d_userptr = obj;

• struct page **pages;
  

• int i;
  

The above line can also be deleted, see below.

...

  if (!obj)
          return;

@@ -398,15 +399,11 @@ static void g2d_userptr_put_dma_addr(struct g2d_data *g2d, dma_unmap_sgtable(to_dma_dev(g2d->drm_dev), g2d_userptr->sgt, DMA_BIDIRECTIONAL, 0);

• pages = frame_vector_pages(g2d_userptr->vec);
  

• if (!IS_ERR(pages)) {
  

•         int i;
  

• for (i = 0; i < g2d_userptr->npages; i++)
  

•         set_page_dirty_lock(g2d_userptr->pages[i]);
  
  

•         for (i = 0; i < frame_vector_count(g2d_userptr->vec); i++)
  

•                 set_page_dirty_lock(pages[i]);
  

• }
  

• put_vaddr_frames(g2d_userptr->vec);
  

• frame_vector_destroy(g2d_userptr->vec);
  

• unpin_user_pages(g2d_userptr->pages, g2d_userptr->npages);
  

• kvfree(g2d_userptr->pages);
  

You can avoid writing your own loop, and just simplify the whole thing down to two lines:

    unpin_user_pages_dirty_lock(g2d_userptr->pages, g2d_userptr->npages,
                                true);
    kvfree(g2d_userptr->pages);

Oh nice, this is neat. I'll also roll it out in the habanalabs patch, that has the same thing. Well almost, it only uses set_page_dirty, not the _lock variant. But I have no idea whether that matters or not? -Daniel

...

  if (!g2d_userptr->out_of_list)
          list_del_init(&g2d_userptr->list);

@@ -474,35 +471,34 @@ static dma_addr_t *g2d_userptr_get_dma_addr(struct g2d_data *g2d, offset = userptr & ~PAGE_MASK; end = PAGE_ALIGN(userptr + size); npages = (end - start) >> PAGE_SHIFT;

• g2d_userptr->vec = frame_vector_create(npages);
  

• if (!g2d_userptr->vec) {
  

• g2d_userptr->pages = kvmalloc_array(npages, sizeof(*g2d_userptr->pages),
  

•                                     GFP_KERNEL);
  

• if (!g2d_userptr->pages) {
          ret = -ENOMEM;
          goto err_free;
  }
  
  

• ret = get_vaddr_frames(start, npages, FOLL_FORCE | FOLL_WRITE,
  

•         g2d_userptr->vec);
  

• ret = pin_user_pages_fast(start, npages, FOLL_FORCE | FOLL_WRITE,
  

•                           g2d_userptr->pages);
  if (ret != npages) {
          DRM_DEV_ERROR(g2d->dev,
                        "failed to get user pages from userptr.\n");
          if (ret < 0)
  

•                 goto err_destroy_framevec;
  

•         ret = -EFAULT;
  

•         goto err_put_framevec;
  

• }
  

• if (frame_vector_to_pages(g2d_userptr->vec) < 0) {
  

•                 goto err_destroy_pages;
  

•         npages = ret;
          ret = -EFAULT;
  

•         goto err_put_framevec;
  

•         goto err_unpin_pages;
  }
  

• g2d_userptr->npages = npages;
  
  sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
  if (!sgt) {
          ret = -ENOMEM;
  

•         goto err_put_framevec;
  

•         goto err_unpin_pages;
  }
  
  ret = sg_alloc_table_from_pages(sgt,
  

•                                 frame_vector_pages(g2d_userptr->vec),
  

•                                 g2d_userptr->pages,
                                  npages, offset, size, GFP_KERNEL);
  if (ret < 0) {
          DRM_DEV_ERROR(g2d->dev, "failed to get sgt from pages.\n");
  

@@ -538,11 +534,11 @@ static dma_addr_t *g2d_userptr_get_dma_addr(struct g2d_data *g2d, err_free_sgt: kfree(sgt);

-err_put_framevec:

• put_vaddr_frames(g2d_userptr->vec);
  

+err_unpin_pages:

• unpin_user_pages(g2d_userptr->pages, npages);
  
  

-err_destroy_framevec:

• frame_vector_destroy(g2d_userptr->vec);
  

+err_destroy_pages:

• kvfree(g2d_userptr->pages);
  
  

  err_free: kfree(g2d_userptr);

The rest all looks good, you've avoided the usual API pitfalls. :)

thanks,

John Hubbard NVIDIA

On Wed, Oct 7, 2020 at 11:37 PM John Hubbard jhubbard@nvidia.com wrote:

On 10/7/20 2:32 PM, Daniel Vetter wrote:

...

On Wed, Oct 7, 2020 at 10:33 PM John Hubbard jhubbard@nvidia.com wrote:

...

On 10/7/20 9:44 AM, Daniel Vetter wrote:

...

...

...

...

@@ -398,15 +399,11 @@ static void g2d_userptr_put_dma_addr(struct g2d_data *g2d, dma_unmap_sgtable(to_dma_dev(g2d->drm_dev), g2d_userptr->sgt, DMA_BIDIRECTIONAL, 0);

• pages = frame_vector_pages(g2d_userptr->vec);
  

• if (!IS_ERR(pages)) {
  

•         int i;
  

• for (i = 0; i < g2d_userptr->npages; i++)
  

•         set_page_dirty_lock(g2d_userptr->pages[i]);
  
  

•         for (i = 0; i < frame_vector_count(g2d_userptr->vec); i++)
  

•                 set_page_dirty_lock(pages[i]);
  

• }
  

• put_vaddr_frames(g2d_userptr->vec);
  

• frame_vector_destroy(g2d_userptr->vec);
  

• unpin_user_pages(g2d_userptr->pages, g2d_userptr->npages);
  

• kvfree(g2d_userptr->pages);
  

You can avoid writing your own loop, and just simplify the whole thing down to two lines:

     unpin_user_pages_dirty_lock(g2d_userptr->pages, g2d_userptr->npages,
                                 true);
     kvfree(g2d_userptr->pages);

Oh nice, this is neat. I'll also roll it out in the habanalabs patch, that has the same thing. Well almost, it only uses set_page_dirty, not the _lock variant. But I have no idea whether that matters or not?

It matters. And invariably, call sites that use set_page_dirty() instead of set_page_dirty_lock() were already wrong. Which is why I never had to provide anything like "unpin_user_pages_dirty (not locked)".

Although in habanalabs case, I just reviewed patch 3 and I think they *were* correctly using set_page_dirty_lock()...

Yeah I mixed that up with some other code I read, habanalabs is using _lock. I have seen a pile of gup/pup code though that only uses set_page_dirty. And looking around I did not really parse the comment above set_page_dirty(). I guess just using the _lock variant shouldn't hurt too much. I've found a comment though from the infiniband umem notifier that it's sometimes called with the page locked, and sometimes not, so life is complicated there. But how it avoids races I didn't understand. -Daniel

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

On 10/7/20 9:44 AM, Daniel Vetter wrote:

The exynos g2d interface is very unusual, but it looks like the userptr objects are persistent. Hence they need FOLL_LONGTERM.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Jason Gunthorpe jgg@ziepe.ca Cc: Inki Dae inki.dae@samsung.com Cc: Joonyoung Shim jy0922.shim@samsung.com Cc: Seung-Woo Kim sw0312.kim@samsung.com Cc: Kyungmin Park kyungmin.park@samsung.com Cc: Kukjin Kim kgene@kernel.org Cc: Krzysztof Kozlowski krzk@kernel.org Cc: Andrew Morton akpm@linux-foundation.org Cc: John Hubbard jhubbard@nvidia.com Cc: Jérôme Glisse jglisse@redhat.com Cc: Jan Kara jack@suse.cz Cc: Dan Williams dan.j.williams@intel.com Cc: linux-mm@kvack.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-samsung-soc@vger.kernel.org Cc: linux-media@vger.kernel.org

---

drivers/gpu/drm/exynos/exynos_drm_g2d.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c b/drivers/gpu/drm/exynos/exynos_drm_g2d.c index c83f6faac9de..514fd000feb1 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c +++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c @@ -478,7 +478,8 @@ static dma_addr_t *g2d_userptr_get_dma_addr(struct g2d_data *g2d, goto err_free; }

• ret = pin_user_pages_fast(start, npages, FOLL_FORCE | FOLL_WRITE,

• ret = pin_user_pages_fast(start, npages,

• 		  FOLL_FORCE | FOLL_WRITE | FOLL_LONGTERM,
  	  g2d_userptr->pages);
  

  if (ret != npages) { DRM_DEV_ERROR(g2d->dev,

Looks good from a pin_user_pages_fast() point of view. I'm of course not a exynos developer, so we still need a look from one of those, ideally, but:

Reviewed-by: John Hubbard jhubbard@nvidia.com

thanks,

On 10/7/20 9:44 AM, Daniel Vetter wrote: ...

@@ -1414,15 +1410,10 @@ void hl_unpin_host_memory(struct hl_device *hdev, struct hl_userptr *userptr) userptr->sgt->nents, userptr->dir);

• pages = frame_vector_pages(userptr->vec);
• if (!IS_ERR(pages)) {

• int i;
  

• for (i = 0; i < frame_vector_count(userptr->vec); i++)
  

• 	set_page_dirty_lock(pages[i]);
  

• }
• put_vaddr_frames(userptr->vec);
• frame_vector_destroy(userptr->vec);

• for (i = 0; i < userptr->npages; i++)

• set_page_dirty_lock(userptr->pages[i]);
  

• unpin_user_pages(userptr->pages, userptr->npages);
• kvfree(userptr->pages);

Same thing here as in patch 1: you can further simplify by using unpin_user_pages_dirty_lock().

list_del(&userptr->job_node);

thanks,

On 10/7/20 9:44 AM, Daniel Vetter wrote:

These are persistent, not just for the duration of a dma operation.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Jason Gunthorpe jgg@ziepe.ca Cc: Andrew Morton akpm@linux-foundation.org Cc: John Hubbard jhubbard@nvidia.com Cc: Jérôme Glisse jglisse@redhat.com Cc: Jan Kara jack@suse.cz Cc: Dan Williams dan.j.williams@intel.com Cc: linux-mm@kvack.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-samsung-soc@vger.kernel.org Cc: linux-media@vger.kernel.org Cc: Oded Gabbay oded.gabbay@gmail.com Cc: Omer Shpigelman oshpigelman@habana.ai Cc: Ofir Bitton obitton@habana.ai Cc: Tomer Tayar ttayar@habana.ai Cc: Moti Haimovski mhaimovski@habana.ai Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Pawel Piskorski ppiskorski@habana.ai

---

drivers/misc/habanalabs/common/memory.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/habanalabs/common/memory.c b/drivers/misc/habanalabs/common/memory.c index ef89cfa2f95a..94bef8faa82a 100644 --- a/drivers/misc/habanalabs/common/memory.c +++ b/drivers/misc/habanalabs/common/memory.c @@ -1288,7 +1288,8 @@ static int get_user_memory(struct hl_device *hdev, u64 addr, u64 size, return -ENOMEM; }

• rc = pin_user_pages_fast(start, npages, FOLL_FORCE | FOLL_WRITE,

• rc = pin_user_pages_fast(start, npages,

• 		 FOLL_FORCE | FOLL_WRITE | FOLL_LONGTERM,
  	 userptr->pages);
  
  

  if (rc != npages) {

Again, from a pin_user_pages_fast() point of view, and not being at all familiar with the habana driver (but their use of this really does seem clearly _LONGTERM!):

Reviewed-by: John Hubbard jhubbard@nvidia.com

thanks,

On Wed, Oct 7, 2020 at 11:13 PM John Hubbard jhubbard@nvidia.com wrote:

On 10/7/20 9:44 AM, Daniel Vetter wrote:

...

This is used by media/videbuf2 for persistent dma mappings, not just for a single dma operation and then freed again, so needs FOLL_LONGTERM.

Unfortunately current pup_locked doesn't support FOLL_LONGTERM due to locking issues. Rework the code to pull the pup path out from the mmap_sem critical section as suggested by Jason.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Jason Gunthorpe jgg@ziepe.ca Cc: Pawel Osciak pawel@osciak.com Cc: Marek Szyprowski m.szyprowski@samsung.com Cc: Kyungmin Park kyungmin.park@samsung.com Cc: Tomasz Figa tfiga@chromium.org Cc: Mauro Carvalho Chehab mchehab@kernel.org Cc: Andrew Morton akpm@linux-foundation.org Cc: John Hubbard jhubbard@nvidia.com Cc: Jérôme Glisse jglisse@redhat.com Cc: Jan Kara jack@suse.cz Cc: Dan Williams dan.j.williams@intel.com Cc: linux-mm@kvack.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-samsung-soc@vger.kernel.org Cc: linux-media@vger.kernel.org

---

mm/frame_vector.c | 36 +++++++++++------------------------- 1 file changed, 11 insertions(+), 25 deletions(-)

diff --git a/mm/frame_vector.c b/mm/frame_vector.c index 10f82d5643b6..39db520a51dc 100644 --- a/mm/frame_vector.c +++ b/mm/frame_vector.c @@ -38,7 +38,6 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames, struct vm_area_struct *vma; int ret = 0; int err;

• int locked;
  
  if (nr_frames == 0)
          return 0;
  

@@ -48,35 +47,22 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames,

  start = untagged_addr(start);

• ret = pin_user_pages_fast(start, nr_frames,
  

•                           FOLL_FORCE | FOLL_WRITE | FOLL_LONGTERM,
  

•                           (struct page **)(vec->ptrs));
  

• if (ret > 0) {
  

•         vec->got_ref = true;
  

•         vec->is_pfns = false;
  

•         goto out_unlocked;
  

• }
  

This part looks good, and changing to _fast is a potential performance improvement, too.

...

• mmap_read_lock(mm);
  

• locked = 1;
  vma = find_vma_intersection(mm, start, start + 1);
  if (!vma) {
          ret = -EFAULT;
          goto out;
  }
  
  

• /*
  

•  * While get_vaddr_frames() could be used for transient (kernel
  

•  * controlled lifetime) pinning of memory pages all current
  

•  * users establish long term (userspace controlled lifetime)
  

•  * page pinning. Treat get_vaddr_frames() like
  

•  * get_user_pages_longterm() and disallow it for filesystem-dax
  

•  * mappings.
  

•  */
  

• if (vma_is_fsdax(vma)) {
  

•         ret = -EOPNOTSUPP;
  

•         goto out;
  

• }
  

Are you sure we don't need to check vma_is_fsdax() anymore?

Since FOLL_LONGTERM checks for this and can only return struct page backed memory, and explicitly excludes VM_IO | VM_PFNMAP, was assuming this is not needed for follow_pfn. And the get_user_pages_locked this used back then didn't have the same check, hence why it was added (and FOLL_LONGTERM still doesn't work for the _locked versions, as you pointed out on the last round of this discussion).

But now that you're asking, I have no idea whether fsdax vma can also be of VM_IO | VM_PFNMAP type. I'm not seeing that set anywhere in fs/dax.c, but that says nothing :-)

Dan, you added this check originally, do we need it for VM_SPECIAL vmas too?

Thanks, Daniel

...

• if (!(vma->vm_flags & (VM_IO | VM_PFNMAP))) {
  

•         vec->got_ref = true;
  

•         vec->is_pfns = false;
  

•         ret = pin_user_pages_locked(start, nr_frames,
  

•                 gup_flags, (struct page **)(vec->ptrs), &locked);
  

•         goto out;
  

• }
  

• vec->got_ref = false;
  vec->is_pfns = true;
  do {
  

@@ -101,8 +87,8 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames, vma = find_vma_intersection(mm, start, start + 1); } while (vma && vma->vm_flags & (VM_IO | VM_PFNMAP)); out:

• if (locked)
  

•         mmap_read_unlock(mm);
  

• mmap_read_unlock(mm);
  

+out_unlocked: if (!ret) ret = -EFAULT; if (ret > 0)

All of the error handling still looks accurate there.

thanks,

John Hubbard NVIDIA

On 10/7/20 9:44 AM, Daniel Vetter wrote:

It's the only user. This also garbage collects the CONFIG_FRAME_VECTOR symbol from all over the tree (well just one place, somehow omap media driver still had this in its Kconfig, despite not using it).

Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Jason Gunthorpe jgg@ziepe.ca Cc: Pawel Osciak pawel@osciak.com Cc: Marek Szyprowski m.szyprowski@samsung.com Cc: Kyungmin Park kyungmin.park@samsung.com Cc: Tomasz Figa tfiga@chromium.org Cc: Mauro Carvalho Chehab mchehab@kernel.org Cc: Andrew Morton akpm@linux-foundation.org Cc: John Hubbard jhubbard@nvidia.com Cc: Jérôme Glisse jglisse@redhat.com Cc: Jan Kara jack@suse.cz Cc: Dan Williams dan.j.williams@intel.com Cc: linux-mm@kvack.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-samsung-soc@vger.kernel.org Cc: linux-media@vger.kernel.org Cc: Daniel Vetter daniel.vetter@ffwll.ch

---

Failed to spot any problems here. :)

Reviewed-by: John Hubbard jhubbard@nvidia.com

thanks,

On Thu, Oct 8, 2020 at 2:44 AM John Hubbard jhubbard@nvidia.com wrote:

On 10/7/20 9:44 AM, Daniel Vetter wrote:

...

Way back it was a reasonable assumptions that iomem mappings never change the pfn range they point at. But this has changed:

• gpu drivers dynamically manage their memory nowadays, invalidating ptes with unmap_mapping_range when buffers get moved

• contiguous dma allocations have moved from dedicated carvetouts to

s/carvetouts/carveouts/

...

cma regions. This means if we miss the unmap the pfn might contain pagecache or anon memory (well anything allocated with GFP_MOVEABLE)

• even /dev/mem now invalidates mappings when the kernel requests that iomem region when CONFIG_IO_STRICT_DEVMEM is set, see 3234ac664a87 ("/dev/mem: Revoke mappings when a driver claims the region")

Thanks for putting these references into the log, it's very helpful. ...

...

diff --git a/mm/memory.c b/mm/memory.c index fcfc4ca36eba..8d467e23b44e 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -4873,28 +4873,68 @@ int follow_phys(struct vm_area_struct *vma, return ret; }

+/**

• • generic_access_phys - generic implementation for iomem mmap access
• • @vma: the vma to access
• • @addr: userspace addres, not relative offset within @vma
• • @buf: buffer to read/write
• • @len: length of transfer
• • @write: set to FOLL_WRITE when writing, otherwise reading
• • This is a generic implementation for &vm_operations_struct.access for an
• • iomem mapping. This callback is used by access_process_vm() when the @vma is
• • not page based.
• */ int generic_access_phys(struct vm_area_struct *vma, unsigned long addr, void *buf, int len, int write) { resource_size_t phys_addr; unsigned long prot = 0; void __iomem *maddr;

• pte_t *ptep, pte;
  

• spinlock_t *ptl;
  int offset = addr & (PAGE_SIZE-1);
  

• int ret = -EINVAL;
  

• if (!(vma->vm_flags & (VM_IO | VM_PFNMAP)))
  

•         return -EINVAL;
  

+retry:

• if (follow_pte(vma->vm_mm, addr, &ptep, &ptl))
  

•         return -EINVAL;
  

• pte = *ptep;
  

• pte_unmap_unlock(ptep, ptl);
  
  

• if (follow_phys(vma, addr, write, &prot, &phys_addr))
  

• prot = pgprot_val(pte_pgprot(pte));
  

• phys_addr = (resource_size_t)pte_pfn(pte) << PAGE_SHIFT;
  

• if ((write & FOLL_WRITE) && !pte_write(pte))
          return -EINVAL;
  
  maddr = ioremap_prot(phys_addr, PAGE_ALIGN(len + offset), prot);
  if (!maddr)
          return -ENOMEM;
  
  

• if (follow_pte(vma->vm_mm, addr, &ptep, &ptl))
  

•         goto out_unmap;
  

• if (pte_same(pte, *ptep)) {
  

The ioremap area is something I'm sorta new to, so a newbie question: is it possible for the same pte to already be there, ever? If so, we be stuck in an infinite loop here. I'm sure that's not the case, but it's not yet obvious to me why it's impossible. Resource reservations maybe?

It's just buggy, it should be !pte_same. And I need to figure out how to test this I guess. -Daniel

On Wed, 7 Oct 2020 18:44:21 +0200 Daniel Vetter daniel.vetter@ffwll.ch wrote:

Way back it was a reasonable assumptions that iomem mappings never change the pfn range they point at. But this has changed:

• gpu drivers dynamically manage their memory nowadays, invalidating

ptes with unmap_mapping_range when buffers get moved

• contiguous dma allocations have moved from dedicated carvetouts to

cma regions. This means if we miss the unmap the pfn might contain pagecache or anon memory (well anything allocated with GFP_MOVEABLE)

• even /dev/mem now invalidates mappings when the kernel requests that

iomem region when CONFIG_IO_STRICT_DEVMEM is set, see 3234ac664a87 ("/dev/mem: Revoke mappings when a driver claims the region")

Accessing pfns obtained from ptes without holding all the locks is therefore no longer a good idea. Fix this.

Since zpci_memcpy_from|toio seems to not do anything nefarious with locks we just need to open code get_pfn and follow_pfn and make sure we drop the locks only after we've done. The write function also needs the copy_from_user move, since we can't take userspace faults while holding the mmap sem.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Jason Gunthorpe jgg@ziepe.ca Cc: Dan Williams dan.j.williams@intel.com Cc: Kees Cook keescook@chromium.org Cc: Andrew Morton akpm@linux-foundation.org Cc: John Hubbard jhubbard@nvidia.com Cc: Jérôme Glisse jglisse@redhat.com Cc: Jan Kara jack@suse.cz Cc: Dan Williams dan.j.williams@intel.com Cc: linux-mm@kvack.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-samsung-soc@vger.kernel.org Cc: linux-media@vger.kernel.org Cc: Niklas Schnelle schnelle@linux.ibm.com Cc: Gerald Schaefer gerald.schaefer@linux.ibm.com Cc: linux-s390@vger.kernel.org

---

arch/s390/pci/pci_mmio.c | 98 +++++++++++++++++++++++----------------- 1 file changed, 57 insertions(+), 41 deletions(-)

Looks good, thanks. Also survived some basic function test. Only some minor nitpick, see below.

Reviewed-by: Gerald Schaefer gerald.schaefer@linux.ibm.com

diff --git a/arch/s390/pci/pci_mmio.c b/arch/s390/pci/pci_mmio.c index 401cf670a243..4d194cb09372 100644 --- a/arch/s390/pci/pci_mmio.c +++ b/arch/s390/pci/pci_mmio.c @@ -119,33 +119,15 @@ static inline int __memcpy_toio_inuser(void __iomem *dst, return rc; }

-static long get_pfn(unsigned long user_addr, unsigned long access,

•     unsigned long *pfn)
  

-{

• struct vm_area_struct *vma;
• long ret;
• mmap_read_lock(current->mm);
• ret = -EINVAL;
• vma = find_vma(current->mm, user_addr);
• if (!vma)

• goto out;
  

• ret = -EACCES;
• if (!(vma->vm_flags & access))

• goto out;
  

• ret = follow_pfn(vma, user_addr, pfn);

-out:

• mmap_read_unlock(current->mm);
• return ret;

-}

SYSCALL_DEFINE3(s390_pci_mmio_write, unsigned long, mmio_addr, const void __user *, user_buffer, size_t, length) { u8 local_buf[64]; void __iomem *io_addr; void *buf;

• unsigned long pfn;

• struct vm_area_struct *vma;

• pte_t *ptep;

• spinlock_t *ptl; long ret;

  if (!zpci_is_enabled())

@@ -158,7 +140,7 @@ SYSCALL_DEFINE3(s390_pci_mmio_write, unsigned long, mmio_addr, * We only support write access to MIO capable devices if we are on * a MIO enabled system. Otherwise we would have to check for every * address if it is a special ZPCI_ADDR and would have to do

• * a get_pfn() which we don't need for MIO capable devices.  Currently
  

• * a pfn lookup which we don't need for MIO capable devices.  Currently
  

  • ISM devices are the only devices without MIO support and there is no
  • known need for accessing these from userspace.
  */

@@ -176,21 +158,37 @@ SYSCALL_DEFINE3(s390_pci_mmio_write, unsigned long, mmio_addr, } else buf = local_buf;

• ret = get_pfn(mmio_addr, VM_WRITE, &pfn);

• ret = -EFAULT;
• if (copy_from_user(buf, user_buffer, length))

• goto out_free;
  

• mmap_read_lock(current->mm);
• ret = -EINVAL;
• vma = find_vma(current->mm, mmio_addr);
• if (!vma)

• goto out_unlock_mmap;
  

• ret = -EACCES;
• if (!(vma->vm_flags & VM_WRITE))

• goto out_unlock_mmap;
  

• if (!(vma->vm_flags & (VM_IO | VM_PFNMAP)))

• goto out_unlock_mmap;
  

That check for VM_IO | VM_PFNMAP was previously hidden inside follow_pfn(), and that would have returned -EINVAL in this case. With your change, we now return -EACCES. Not sure how important that is, but it feels wrong. Maybe move the VM_IO | VM_PFNMAP check up, before the ret = -EACCES?

[...]

@@ -306,22 +306,38 @@ SYSCALL_DEFINE3(s390_pci_mmio_read, unsigned long, mmio_addr, buf = local_buf; }

• ret = get_pfn(mmio_addr, VM_READ, &pfn);

• mmap_read_lock(current->mm);
• ret = -EINVAL;
• vma = find_vma(current->mm, mmio_addr);
• if (!vma)

• goto out_unlock_mmap;
  

• ret = -EACCES;
• if (!(vma->vm_flags & VM_WRITE))

• goto out_unlock_mmap;
  

• if (!(vma->vm_flags & (VM_IO | VM_PFNMAP)))

• goto out_unlock_mmap;
  

Same here with VM_IO | VM_PFNMAP and -EINVAL.

On Wed, Oct 7, 2020 at 8:41 PM Bjorn Helgaas helgaas@kernel.org wrote:

Capitalize subject, like other patches in this series and previous drivers/pci history.

On Wed, Oct 07, 2020 at 06:44:23PM +0200, Daniel Vetter wrote:

...

Since 3234ac664a87 ("/dev/mem: Revoke mappings when a driver claims the region") /dev/kmem zaps ptes when the kernel requests exclusive acccess to an iomem region. And with CONFIG_IO_STRICT_DEVMEM, this is the default for all driver uses.

Except there's two more ways to access pci bars: sysfs and proc mmap support. Let's plug that hole.

s/pci/PCI/ in commit logs and comments.

...

For revoke_devmem() to work we need to link our vma into the same address_space, with consistent vma->vm_pgoff. ->pgoff is already adjusted, because that's how (io_)remap_pfn_range works, but for the mapping we need to adjust vma->vm_file->f_mapping. Usually that's done at ->open time, but that's a bit tricky here with all the entry points and arch code. So instead create a fake file and adjust vma->vm_file.

Note this only works for ARCH_GENERIC_PCI_MMAP_RESOURCE. But that seems to be a subset of architectures support STRICT_DEVMEM, so we should be good.

The only difference in access checks left is that sysfs pci mmap does not check for CAP_RAWIO. But I think that makes some sense compared to /dev/mem and proc, where one file gives you access to everything and no ownership applies.

...

--- a/drivers/char/mem.c +++ b/drivers/char/mem.c @@ -810,6 +810,7 @@ static loff_t memory_lseek(struct file *file, loff_t offset, int orig) }

static struct inode *devmem_inode; +static struct vfsmount *devmem_vfs_mount;

#ifdef CONFIG_IO_STRICT_DEVMEM void revoke_devmem(struct resource *res) @@ -843,6 +844,20 @@ void revoke_devmem(struct resource *res)

  unmap_mapping_range(inode->i_mapping, res->start, resource_size(res), 1);

}

+struct file *devmem_getfile(void) +{

• struct file *file;
  

• file = alloc_file_pseudo(devmem_inode, devmem_vfs_mount, "devmem",
  

•                          O_RDWR, &kmem_fops);
  

• if (IS_ERR(file))
  

•         return NULL;
  

• file->f_mapping = devmem_indoe->i_mapping;
  

"devmem_indoe"? Obviously not compiled, I guess?

Yeah apologies, I forgot to compile this with CONFIG_IO_STRICT_DEVMEM set. The entire series is more rfc about the overall problem really, I need to also figure out how to even this this somehow. I guess there's nothing really ready made here? -Daniel

...

--- a/include/linux/ioport.h +++ b/include/linux/ioport.h @@ -304,8 +304,10 @@ struct resource *request_free_mem_region(struct resource *base,

#ifdef CONFIG_IO_STRICT_DEVMEM void revoke_devmem(struct resource *res); +struct file *devm_getfile(void); #else static inline void revoke_devmem(struct resource *res) { }; +static inline struct file *devmem_getfile(void) { return NULL; };

I guess these names are supposed to match?

...

#endif

#endif /* __ASSEMBLY__ */

2.28.0

On Wed, Oct 7, 2020 at 7:36 PM Jason Gunthorpe jgg@ziepe.ca wrote:

On Wed, Oct 07, 2020 at 06:44:24PM +0200, Daniel Vetter wrote:

...

Way back it was a reasonable assumptions that iomem mappings never change the pfn range they point at. But this has changed:

• gpu drivers dynamically manage their memory nowadays, invalidating

ptes with unmap_mapping_range when buffers get moved

• contiguous dma allocations have moved from dedicated carvetouts to

cma regions. This means if we miss the unmap the pfn might contain pagecache or anon memory (well anything allocated with GFP_MOVEABLE)

• even /dev/mem now invalidates mappings when the kernel requests that

iomem region when CONFIG_IO_STRICT_DEVMEM is set, see 3234ac664a87 ("/dev/mem: Revoke mappings when a driver claims the region")

Accessing pfns obtained from ptes without holding all the locks is therefore no longer a good idea.

Unfortunately there's some users where this is not fixable (like v4l userptr of iomem mappings) or involves a pile of work (vfio type1 iommu). For now annotate these as unsafe and splat appropriately.

This patch adds an unsafe_follow_pfn, which later patches will then roll out to all appropriate places.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Jason Gunthorpe jgg@ziepe.ca Cc: Kees Cook keescook@chromium.org Cc: Dan Williams dan.j.williams@intel.com Cc: Andrew Morton akpm@linux-foundation.org Cc: John Hubbard jhubbard@nvidia.com Cc: Jérôme Glisse jglisse@redhat.com Cc: Jan Kara jack@suse.cz Cc: Dan Williams dan.j.williams@intel.com Cc: linux-mm@kvack.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-samsung-soc@vger.kernel.org Cc: linux-media@vger.kernel.org Cc: kvm@vger.kernel.org

---

include/linux/mm.h | 2 ++ mm/memory.c | 32 +++++++++++++++++++++++++++++++- mm/nommu.c | 17 +++++++++++++++++ security/Kconfig | 13 +++++++++++++ 4 files changed, 63 insertions(+), 1 deletion(-)

Makes sense to me.

I wonder if we could change the original follow_pfn to require the ptep and then lockdep_assert_held() it against the page table lock?

The safe variant with the pagetable lock is follow_pte_pmd. The only way to make follow_pfn safe is if you have an mmu notifier and corresponding retry logic. That is not covered by lockdep (it would splat if we annotate the retry side), so I'm not sure how you'd check for that?

Checking for ptep lock doesn't work here, since the one leftover safe user of this (kvm) doesn't need that at all, because it has the mmu notifier.

Also follow_pte_pmd will splat with lockdep if you get it wrong, since the function leaves you with the right ptlock lock when it returns. If you forget to unlock that, lockdep will complain.

So I think we're as good as it gets, since I really have no idea how to make sure follow_pfn callers do have an mmu notifier registered.

...

+int unsafe_follow_pfn(struct vm_area_struct *vma, unsigned long address,

• unsigned long *pfn)
  

+{ +#ifdef CONFIG_STRICT_FOLLOW_PFN

• pr_info("unsafe follow_pfn usage rejected, see
  

CONFIG_STRICT_FOLLOW_PFN\n");

Wonder if we can print something useful here, like the current PID/process name?

Yeah adding comm/pid here makes sense.

...

diff --git a/security/Kconfig b/security/Kconfig index 7561f6f99f1d..48945402e103 100644 --- a/security/Kconfig +++ b/security/Kconfig @@ -230,6 +230,19 @@ config STATIC_USERMODEHELPER_PATH If you wish for all usermode helper programs to be disabled, specify an empty string here (i.e. "").

+config STRICT_FOLLOW_PFN

• bool "Disable unsafe use of follow_pfn"
  

• depends on MMU
  

I would probably invert this CONFIG_ALLOW_UNSAFE_FOLLOW_PFN default n

I've followed the few other CONFIG_STRICT_FOO I've seen, which are all explicit enables and default to "do not break uapi, damn the (security) bugs". Which is I think how this should be done. It is in the security section though, so hopefully competent distros will enable this all. -Daniel

On Wed, Oct 7, 2020 at 7:39 PM Jason Gunthorpe jgg@ziepe.ca wrote:

On Wed, Oct 07, 2020 at 06:44:26PM +0200, Daniel Vetter wrote:

...

The code seems to stuff these pfns into iommu pts (or something like that, I didn't follow), but there's no mmu_notifier to ensure that access is synchronized with pte updates.

Hence mark these as unsafe. This means that with CONFIG_STRICT_FOLLOW_PFN, these will be rejected.

Real fix is to wire up an mmu_notifier ... somehow. Probably means any invalidate is a fatal fault for this vfio device, but then this shouldn't ever happen if userspace is reasonable.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Jason Gunthorpe jgg@ziepe.ca Cc: Kees Cook keescook@chromium.org Cc: Dan Williams dan.j.williams@intel.com Cc: Andrew Morton akpm@linux-foundation.org Cc: John Hubbard jhubbard@nvidia.com Cc: Jérôme Glisse jglisse@redhat.com Cc: Jan Kara jack@suse.cz Cc: Dan Williams dan.j.williams@intel.com Cc: linux-mm@kvack.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-samsung-soc@vger.kernel.org Cc: linux-media@vger.kernel.org Cc: Alex Williamson alex.williamson@redhat.com Cc: Cornelia Huck cohuck@redhat.com Cc: kvm@vger.kernel.org

---

drivers/vfio/vfio_iommu_type1.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index 5fbf0c1f7433..a4d53f3d0a35 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -421,7 +421,7 @@ static int follow_fault_pfn(struct vm_area_struct *vma, struct mm_struct *mm, { int ret;

• ret = follow_pfn(vma, vaddr, pfn);
  

• ret = unsafe_follow_pfn(vma, vaddr, pfn);
  if (ret) {
          bool unlocked = false;
  
  

@@ -435,7 +435,7 @@ static int follow_fault_pfn(struct vm_area_struct *vma, struct mm_struct *mm, if (ret) return ret;

•         ret = follow_pfn(vma, vaddr, pfn);
  

•         ret = unsafe_follow_pfn(vma, vaddr, pfn);
  }
  

This is actually being commonly used, so it needs fixing.

When I talked to Alex about this last we had worked out a patch series that adds a test on vm_ops that the vma came from vfio in the first place. The VMA's created by VFIO are 'safe' as the PTEs are never changed.

Hm, but wouldn't need that the semi-nasty vma_open trick to make sure that vma doesn't untimely disappear? Or is the idea to look up the underlying vfio object, and refcount that directly? -Daniel