kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v7 00/17] follow_pfn and other iomap races
@ 2020-11-27 16:41 Daniel Vetter
  2020-11-27 16:41 ` [PATCH v7 01/17] drm/exynos: Stop using frame_vector helpers Daniel Vetter
                   ` (17 more replies)
  0 siblings, 18 replies; 32+ messages in thread
From: Daniel Vetter @ 2020-11-27 16:41 UTC (permalink / raw)
  To: DRI Development, LKML
  Cc: kvm, linux-mm, linux-arm-kernel, linux-samsung-soc, linux-media,
	Daniel Vetter

Hi all

Another update of my patch series to clamp down a bunch of races and gaps
around follow_pfn and other access to iomem mmaps. Previous version:

v1: https://lore.kernel.org/dri-devel/20201007164426.1812530-1-daniel.vetter@ffwll.ch/
v2: https://lore.kernel.org/dri-devel/20201009075934.3509076-1-daniel.vetter@ffwll.ch
v3: https://lore.kernel.org/dri-devel/20201021085655.1192025-1-daniel.vetter@ffwll.ch/
v4: https://lore.kernel.org/dri-devel/20201026105818.2585306-1-daniel.vetter@ffwll.ch/
v5: https://lore.kernel.org/dri-devel/20201030100815.2269-1-daniel.vetter@ffwll.ch/
v6: https://lore.kernel.org/dri-devel/20201119144146.1045202-1-daniel.vetter@ffwll.ch/

And the discussion that sparked this journey:

https://lore.kernel.org/dri-devel/20201007164426.1812530-1-daniel.vetter@ffwll.ch/

I think the first 12 patches are ready for landing. The parts starting
with "mm: Add unsafe_follow_pfn" probably need more baking time.

Andrew, can you please pick these up, or do you prefer I do a topic branch
and send them to Linus directly in the next merge window?

Changes in v7:
- more acks/reviews
- reordered with the ready pieces at the front
- simplified the new follow_pfn function as Jason suggested

Changes in v6:
- Tested v4l userptr as Tomasz suggested. No boom observed
- Added RFC for locking down follow_pfn, per discussion with Christoph and
  Jason.
- Explain why pup_fast is safe in relevant patches, there was a bit a
  confusion when discussing v5.
- Fix up the resource patch, with CONFIG_IO_STRICT_DEVMEM it crashed on
  boot due to an unintended change (reported by John)

Changes in v5:
- Tomasz found some issues in the media patches
- Polish suggested by Christoph for the unsafe_follow_pfn patch

Changes in v4:
- Drop the s390 patch, that was very stand-alone and now queued up to land
  through s390 trees.
- Comment polish per Dan's review.

Changes in v3:
- Bunch of polish all over, no functional changes aside from one barrier
  in the resource code, for consistency.
- A few more r-b tags.

Changes in v2:
- tons of small polish&fixes all over, thanks to all the reviewers who
  spotted issues
- I managed to test at least the generic_access_phys and pci mmap revoke
  stuff with a few gdb sessions using our i915 debug tools (hence now also
  the drm/i915 patch to properly request all the pci bar regions)
- reworked approach for the pci mmap revoke: Infrastructure moved into
  kernel/resource.c, address_space mapping is now set up at open time for
  everyone (which required some sysfs changes). Does indeed look a lot
  cleaner and a lot less invasive than I feared at first.

Coments and review on the remaining bits very much welcome, especially
from the kvm and vfio side.

Cheers, Daniel

Daniel Vetter (17):
  drm/exynos: Stop using frame_vector helpers
  drm/exynos: Use FOLL_LONGTERM for g2d cmdlists
  misc/habana: Stop using frame_vector helpers
  misc/habana: Use FOLL_LONGTERM for userptr
  mm/frame-vector: Use FOLL_LONGTERM
  media: videobuf2: Move frame_vector into media subsystem
  mm: Close race in generic_access_phys
  PCI: Obey iomem restrictions for procfs mmap
  /dev/mem: Only set filp->f_mapping
  resource: Move devmem revoke code to resource framework
  sysfs: Support zapping of binary attr mmaps
  PCI: Revoke mappings like devmem
  mm: Add unsafe_follow_pfn
  media/videobuf1|2: Mark follow_pfn usage as unsafe
  vfio/type1: Mark follow_pfn as unsafe
  kvm: pass kvm argument to follow_pfn callsites
  mm: add mmu_notifier argument to follow_pfn

 arch/powerpc/kvm/book3s_64_mmu_hv.c           |   2 +-
 arch/powerpc/kvm/book3s_64_mmu_radix.c        |   2 +-
 arch/powerpc/kvm/e500_mmu_host.c              |   2 +-
 arch/x86/kvm/mmu/mmu.c                        |   8 +-
 drivers/char/mem.c                            |  86 +-------------
 drivers/gpu/drm/exynos/Kconfig                |   1 -
 drivers/gpu/drm/exynos/exynos_drm_g2d.c       |  48 ++++----
 drivers/media/common/videobuf2/Kconfig        |   1 -
 drivers/media/common/videobuf2/Makefile       |   1 +
 .../media/common/videobuf2}/frame_vector.c    |  57 ++++-----
 .../media/common/videobuf2/videobuf2-memops.c |   3 +-
 drivers/media/platform/omap/Kconfig           |   1 -
 drivers/media/v4l2-core/videobuf-dma-contig.c |   2 +-
 drivers/misc/habanalabs/Kconfig               |   1 -
 drivers/misc/habanalabs/common/habanalabs.h   |   6 +-
 drivers/misc/habanalabs/common/memory.c       |  52 +++-----
 drivers/pci/pci-sysfs.c                       |   4 +
 drivers/pci/proc.c                            |   6 +
 drivers/vfio/vfio_iommu_type1.c               |   4 +-
 fs/sysfs/file.c                               |  11 ++
 include/linux/ioport.h                        |   6 +-
 include/linux/kvm_host.h                      |   9 +-
 include/linux/mm.h                            |  50 +-------
 include/linux/sysfs.h                         |   2 +
 include/media/frame_vector.h                  |  47 ++++++++
 include/media/videobuf2-core.h                |   1 +
 kernel/resource.c                             |  98 ++++++++++++++-
 mm/Kconfig                                    |   3 -
 mm/Makefile                                   |   1 -
 mm/memory.c                                   | 112 +++++++++++++++---
 mm/nommu.c                                    |  16 ++-
 security/Kconfig                              |  13 ++
 virt/kvm/kvm_main.c                           |  56 +++++----
 33 files changed, 413 insertions(+), 299 deletions(-)
 rename {mm => drivers/media/common/videobuf2}/frame_vector.c (84%)
 create mode 100644 include/media/frame_vector.h

-- 
2.29.2


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v7 01/17] drm/exynos: Stop using frame_vector helpers
  2020-11-27 16:41 [PATCH v7 00/17] follow_pfn and other iomap races Daniel Vetter
@ 2020-11-27 16:41 ` Daniel Vetter
  2020-11-27 16:41 ` [PATCH v7 02/17] drm/exynos: Use FOLL_LONGTERM for g2d cmdlists Daniel Vetter
                   ` (16 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Daniel Vetter @ 2020-11-27 16:41 UTC (permalink / raw)
  To: DRI Development, LKML
  Cc: kvm, linux-mm, linux-arm-kernel, linux-samsung-soc, linux-media,
	Daniel Vetter, John Hubbard, Daniel Vetter, Jason Gunthorpe,
	Inki Dae, Joonyoung Shim, Seung-Woo Kim, Kyungmin Park,
	Kukjin Kim, Krzysztof Kozlowski, Andrew Morton,
	Christoph Hellwig, Jérôme Glisse, Jan Kara,
	Dan Williams

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.

Note that pin_user_pages_fast is a safe replacement despite the
seeming lack of checking for vma->vm_flasg & (VM_IO | VM_PFNMAP). Such
ptes are marked with pte_mkspecial (which pup_fast rejects in the
fastpath), and only architectures supporting that support the
pin_user_pages_fast fastpath.

Reviewed-by: John Hubbard <jhubbard@nvidia.com>
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: Christoph Hellwig <hch@infradead.org>
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
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
--
v2: Use unpin_user_pages_dirty_lock (John)
v6: Explain why pup_fast is safe, after discussions with John and
Christoph.
---
 drivers/gpu/drm/exynos/Kconfig          |  1 -
 drivers/gpu/drm/exynos/exynos_drm_g2d.c | 47 +++++++++++--------------
 2 files changed, 20 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/exynos/Kconfig b/drivers/gpu/drm/exynos/Kconfig
index 951d5f708e92..6a251e3aa779 100644
--- a/drivers/gpu/drm/exynos/Kconfig
+++ b/drivers/gpu/drm/exynos/Kconfig
@@ -89,7 +89,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..ecede41af9b9 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,6 @@ static void g2d_userptr_put_dma_addr(struct g2d_data *g2d,
 					bool force)
 {
 	struct g2d_cmdlist_userptr *g2d_userptr = obj;
-	struct page **pages;
 
 	if (!obj)
 		return;
@@ -398,15 +398,9 @@ 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 < 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_dirty_lock(g2d_userptr->pages, g2d_userptr->npages,
+				    true);
+	kvfree(g2d_userptr->pages);
 
 	if (!g2d_userptr->out_of_list)
 		list_del_init(&g2d_userptr->list);
@@ -474,35 +468,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 +531,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);
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v7 02/17] drm/exynos: Use FOLL_LONGTERM for g2d cmdlists
  2020-11-27 16:41 [PATCH v7 00/17] follow_pfn and other iomap races Daniel Vetter
  2020-11-27 16:41 ` [PATCH v7 01/17] drm/exynos: Stop using frame_vector helpers Daniel Vetter
@ 2020-11-27 16:41 ` Daniel Vetter
  2020-11-27 16:41 ` [PATCH v7 03/17] misc/habana: Stop using frame_vector helpers Daniel Vetter
                   ` (15 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Daniel Vetter @ 2020-11-27 16:41 UTC (permalink / raw)
  To: DRI Development, LKML
  Cc: kvm, linux-mm, linux-arm-kernel, linux-samsung-soc, linux-media,
	Daniel Vetter, Daniel Vetter, Jason Gunthorpe, Inki Dae,
	Joonyoung Shim, Seung-Woo Kim, Kyungmin Park, Kukjin Kim,
	Krzysztof Kozlowski, Andrew Morton, John Hubbard,
	Jérôme Glisse, Jan Kara, Dan Williams

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
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 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 ecede41af9b9..1e0c5a7f206e 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
@@ -475,7 +475,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,
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v7 03/17] misc/habana: Stop using frame_vector helpers
  2020-11-27 16:41 [PATCH v7 00/17] follow_pfn and other iomap races Daniel Vetter
  2020-11-27 16:41 ` [PATCH v7 01/17] drm/exynos: Stop using frame_vector helpers Daniel Vetter
  2020-11-27 16:41 ` [PATCH v7 02/17] drm/exynos: Use FOLL_LONGTERM for g2d cmdlists Daniel Vetter
@ 2020-11-27 16:41 ` Daniel Vetter
  2020-11-27 16:41 ` [PATCH v7 04/17] misc/habana: Use FOLL_LONGTERM for userptr Daniel Vetter
                   ` (14 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Daniel Vetter @ 2020-11-27 16:41 UTC (permalink / raw)
  To: DRI Development, LKML
  Cc: kvm, linux-mm, linux-arm-kernel, linux-samsung-soc, linux-media,
	Daniel Vetter, John Hubbard, Oded Gabbay, Daniel Vetter,
	Christoph Hellwig, Jason Gunthorpe, Andrew Morton,
	Jérôme Glisse, Jan Kara, Dan Williams, Oded Gabbay,
	Omer Shpigelman, Ofir Bitton, Tomer Tayar, Moti Haimovski,
	Greg Kroah-Hartman, Pawel Piskorski

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.

Note that pin_user_pages_fast is a safe replacement despite the
seeming lack of checking for vma->vm_flasg & (VM_IO | VM_PFNMAP). Such
ptes are marked with pte_mkspecial (which pup_fast rejects in the
fastpath), and only architectures supporting that support the
pin_user_pages_fast fastpath.

Reviewed-by: John Hubbard <jhubbard@nvidia.com>
Reviewed-by: Oded Gabbay <ogabbay@kernel.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Christoph Hellwig <hch@infradead.org>
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>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
--
v2: Use unpin_user_pages_dirty_lock (John)
v3: Update kerneldoc (Oded)
v6: Explain why pup_fast is safe, after discussions with John and
Christoph.
v7: Remove error message for kmalloc failures, habanalabs doesn't do
that (Oded)
---
 drivers/misc/habanalabs/Kconfig             |  1 -
 drivers/misc/habanalabs/common/habanalabs.h |  6 ++-
 drivers/misc/habanalabs/common/memory.c     | 51 ++++++++-------------
 3 files changed, 22 insertions(+), 36 deletions(-)

diff --git a/drivers/misc/habanalabs/Kconfig b/drivers/misc/habanalabs/Kconfig
index 1640340d3e62..293d79811372 100644
--- a/drivers/misc/habanalabs/Kconfig
+++ b/drivers/misc/habanalabs/Kconfig
@@ -6,7 +6,6 @@
 config HABANA_AI
 	tristate "HabanaAI accelerators (habanalabs)"
 	depends on PCI && HAS_IOMEM
-	select FRAME_VECTOR
 	select GENERIC_ALLOCATOR
 	select HWMON
 	help
diff --git a/drivers/misc/habanalabs/common/habanalabs.h b/drivers/misc/habanalabs/common/habanalabs.h
index 6ed974d2def0..107442a20197 100644
--- a/drivers/misc/habanalabs/common/habanalabs.h
+++ b/drivers/misc/habanalabs/common/habanalabs.h
@@ -921,7 +921,8 @@ struct hl_ctx_mgr {
  * struct hl_userptr - memory mapping chunk information
  * @vm_type: type of the VM.
  * @job_node: linked-list node for hanging the object on the Job's list.
- * @vec: pointer to the frame vector.
+ * @pages: pointer to struct page array
+ * @npages: size of @pages array
  * @sgt: pointer to the scatter-gather table that holds the pages.
  * @dir: for DMA unmapping, the direction must be supplied, so save it.
  * @debugfs_list: node in debugfs list of command submissions.
@@ -932,7 +933,8 @@ struct hl_ctx_mgr {
 struct hl_userptr {
 	enum vm_type_t		vm_type; /* must be first */
 	struct list_head	job_node;
-	struct frame_vector	*vec;
+	struct page		**pages;
+	unsigned int		npages;
 	struct sg_table		*sgt;
 	enum dma_data_direction dir;
 	struct list_head	debugfs_list;
diff --git a/drivers/misc/habanalabs/common/memory.c b/drivers/misc/habanalabs/common/memory.c
index 84227819e4d1..2c59fa869684 100644
--- a/drivers/misc/habanalabs/common/memory.c
+++ b/drivers/misc/habanalabs/common/memory.c
@@ -1291,45 +1291,39 @@ static int get_user_memory(struct hl_device *hdev, u64 addr, u64 size,
 		return -EFAULT;
 	}
 
-	userptr->vec = frame_vector_create(npages);
-	if (!userptr->vec) {
-		dev_err(hdev->dev, "Failed to create frame vector\n");
+	userptr->pages = kvmalloc_array(npages, sizeof(*userptr->pages),
+					GFP_KERNEL);
+	if (!userptr->pages)
 		return -ENOMEM;
-	}
 
-	rc = get_vaddr_frames(start, npages, FOLL_FORCE | FOLL_WRITE,
-				userptr->vec);
+	rc = pin_user_pages_fast(start, npages, FOLL_FORCE | FOLL_WRITE,
+				 userptr->pages);
 
 	if (rc != npages) {
 		dev_err(hdev->dev,
 			"Failed to map host memory, user ptr probably wrong\n");
 		if (rc < 0)
-			goto destroy_framevec;
-		rc = -EFAULT;
-		goto put_framevec;
-	}
-
-	if (frame_vector_to_pages(userptr->vec) < 0) {
-		dev_err(hdev->dev,
-			"Failed to translate frame vector to pages\n");
+			goto destroy_pages;
+		npages = rc;
 		rc = -EFAULT;
-		goto put_framevec;
+		goto put_pages;
 	}
+	userptr->npages = npages;
 
 	rc = sg_alloc_table_from_pages(userptr->sgt,
-					frame_vector_pages(userptr->vec),
-					npages, offset, size, GFP_ATOMIC);
+				       userptr->pages,
+				       npages, offset, size, GFP_ATOMIC);
 	if (rc < 0) {
 		dev_err(hdev->dev, "failed to create SG table from pages\n");
-		goto put_framevec;
+		goto put_pages;
 	}
 
 	return 0;
 
-put_framevec:
-	put_vaddr_frames(userptr->vec);
-destroy_framevec:
-	frame_vector_destroy(userptr->vec);
+put_pages:
+	unpin_user_pages(userptr->pages, npages);
+destroy_pages:
+	kvfree(userptr->pages);
 	return rc;
 }
 
@@ -1415,8 +1409,6 @@ int hl_pin_host_memory(struct hl_device *hdev, u64 addr, u64 size,
  */
 void hl_unpin_host_memory(struct hl_device *hdev, struct hl_userptr *userptr)
 {
-	struct page **pages;
-
 	hl_debugfs_remove_userptr(hdev, userptr);
 
 	if (userptr->dma_mapped)
@@ -1424,15 +1416,8 @@ 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);
+	unpin_user_pages_dirty_lock(userptr->pages, userptr->npages, true);
+	kvfree(userptr->pages);
 
 	list_del(&userptr->job_node);
 
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v7 04/17] misc/habana: Use FOLL_LONGTERM for userptr
  2020-11-27 16:41 [PATCH v7 00/17] follow_pfn and other iomap races Daniel Vetter
                   ` (2 preceding siblings ...)
  2020-11-27 16:41 ` [PATCH v7 03/17] misc/habana: Stop using frame_vector helpers Daniel Vetter
@ 2020-11-27 16:41 ` Daniel Vetter
  2020-11-27 16:41 ` [PATCH v7 05/17] mm/frame-vector: Use FOLL_LONGTERM Daniel Vetter
                   ` (13 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Daniel Vetter @ 2020-11-27 16:41 UTC (permalink / raw)
  To: DRI Development, LKML
  Cc: kvm, linux-mm, linux-arm-kernel, linux-samsung-soc, linux-media,
	Daniel Vetter, Oded Gabbay, Daniel Vetter, Jason Gunthorpe,
	Andrew Morton, John Hubbard, Jérôme Glisse, Jan Kara,
	Dan Williams, Oded Gabbay, Omer Shpigelman, Ofir Bitton,
	Tomer Tayar, Moti Haimovski, Greg Kroah-Hartman, Pawel Piskorski

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

Reviewed-by: Oded Gabbay <ogabbay@kernel.org>
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>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 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 2c59fa869684..0d25ae1d5f3e 100644
--- a/drivers/misc/habanalabs/common/memory.c
+++ b/drivers/misc/habanalabs/common/memory.c
@@ -1296,7 +1296,8 @@ static int get_user_memory(struct hl_device *hdev, u64 addr, u64 size,
 	if (!userptr->pages)
 		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) {
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v7 05/17] mm/frame-vector: Use FOLL_LONGTERM
  2020-11-27 16:41 [PATCH v7 00/17] follow_pfn and other iomap races Daniel Vetter
                   ` (3 preceding siblings ...)
  2020-11-27 16:41 ` [PATCH v7 04/17] misc/habana: Use FOLL_LONGTERM for userptr Daniel Vetter
@ 2020-11-27 16:41 ` Daniel Vetter
  2020-11-27 16:41 ` [PATCH v7 06/17] media: videobuf2: Move frame_vector into media subsystem Daniel Vetter
                   ` (12 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Daniel Vetter @ 2020-11-27 16:41 UTC (permalink / raw)
  To: DRI Development, LKML
  Cc: kvm, linux-mm, linux-arm-kernel, linux-samsung-soc, linux-media,
	Daniel Vetter, Daniel Vetter, Christoph Hellwig, Jason Gunthorpe,
	Pawel Osciak, Marek Szyprowski, Kyungmin Park, Tomasz Figa,
	Mauro Carvalho Chehab, Andrew Morton, John Hubbard,
	Jérôme Glisse, Jan Kara, Dan Williams

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.

By relying entirely on the vma checks in pin_user_pages and follow_pfn
(for vm_flags and vma_is_fsdax) we can also streamline the code a lot.

Note that pin_user_pages_fast is a safe replacement despite the
seeming lack of checking for vma->vm_flasg & (VM_IO | VM_PFNMAP). Such
ptes are marked with pte_mkspecial (which pup_fast rejects in the
fastpath), and only architectures supporting that support the
pin_user_pages_fast fastpath.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Christoph Hellwig <hch@infradead.org>
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
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
--
v2: Streamline the code and further simplify the loop checks (Jason)

v5: Review from Tomasz:
- fix page counting for the follow_pfn case by resetting ret
- drop gup_flags paramater, now unused

v6: Explain why pup_fast is safe, after discussions with John and
Christoph.
---
 .../media/common/videobuf2/videobuf2-memops.c |  3 +-
 include/linux/mm.h                            |  2 +-
 mm/frame_vector.c                             | 53 ++++++-------------
 3 files changed, 19 insertions(+), 39 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-memops.c b/drivers/media/common/videobuf2/videobuf2-memops.c
index 6e9e05153f4e..9dd6c27162f4 100644
--- a/drivers/media/common/videobuf2/videobuf2-memops.c
+++ b/drivers/media/common/videobuf2/videobuf2-memops.c
@@ -40,7 +40,6 @@ struct frame_vector *vb2_create_framevec(unsigned long start,
 	unsigned long first, last;
 	unsigned long nr;
 	struct frame_vector *vec;
-	unsigned int flags = FOLL_FORCE | FOLL_WRITE;
 
 	first = start >> PAGE_SHIFT;
 	last = (start + length - 1) >> PAGE_SHIFT;
@@ -48,7 +47,7 @@ struct frame_vector *vb2_create_framevec(unsigned long start,
 	vec = frame_vector_create(nr);
 	if (!vec)
 		return ERR_PTR(-ENOMEM);
-	ret = get_vaddr_frames(start & PAGE_MASK, nr, flags, vec);
+	ret = get_vaddr_frames(start & PAGE_MASK, nr, vec);
 	if (ret < 0)
 		goto out_destroy;
 	/* We accept only complete set of PFNs */
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 47bff16c182d..29a1941cd255 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1765,7 +1765,7 @@ struct frame_vector {
 struct frame_vector *frame_vector_create(unsigned int nr_frames);
 void frame_vector_destroy(struct frame_vector *vec);
 int get_vaddr_frames(unsigned long start, unsigned int nr_pfns,
-		     unsigned int gup_flags, struct frame_vector *vec);
+		     struct frame_vector *vec);
 void put_vaddr_frames(struct frame_vector *vec);
 int frame_vector_to_pages(struct frame_vector *vec);
 void frame_vector_to_pfns(struct frame_vector *vec);
diff --git a/mm/frame_vector.c b/mm/frame_vector.c
index 10f82d5643b6..f8c34b895c76 100644
--- a/mm/frame_vector.c
+++ b/mm/frame_vector.c
@@ -32,13 +32,12 @@
  * This function takes care of grabbing mmap_lock as necessary.
  */
 int get_vaddr_frames(unsigned long start, unsigned int nr_frames,
-		     unsigned int gup_flags, struct frame_vector *vec)
+		     struct frame_vector *vec)
 {
 	struct mm_struct *mm = current->mm;
 	struct vm_area_struct *vma;
 	int ret = 0;
 	int err;
-	int locked;
 
 	if (nr_frames == 0)
 		return 0;
@@ -48,40 +47,26 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames,
 
 	start = untagged_addr(start);
 
-	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;
-	}
-
-	if (!(vma->vm_flags & (VM_IO | VM_PFNMAP))) {
+	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;
-		ret = pin_user_pages_locked(start, nr_frames,
-			gup_flags, (struct page **)(vec->ptrs), &locked);
-		goto out;
+		goto out_unlocked;
 	}
 
+	mmap_read_lock(mm);
 	vec->got_ref = false;
 	vec->is_pfns = true;
+	ret = 0;
 	do {
 		unsigned long *nums = frame_vector_pfns(vec);
 
+		vma = find_vma_intersection(mm, start, start + 1);
+		if (!vma)
+			break;
+
 		while (ret < nr_frames && start + PAGE_SIZE <= vma->vm_end) {
 			err = follow_pfn(vma, start, &nums[ret]);
 			if (err) {
@@ -92,17 +77,13 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames,
 			start += PAGE_SIZE;
 			ret++;
 		}
-		/*
-		 * We stop if we have enough pages or if VMA doesn't completely
-		 * cover the tail page.
-		 */
-		if (ret >= nr_frames || start < vma->vm_end)
+		/* Bail out if VMA doesn't completely cover the tail page. */
+		if (start < vma->vm_end)
 			break;
-		vma = find_vma_intersection(mm, start, start + 1);
-	} while (vma && vma->vm_flags & (VM_IO | VM_PFNMAP));
+	} while (ret < nr_frames);
 out:
-	if (locked)
-		mmap_read_unlock(mm);
+	mmap_read_unlock(mm);
+out_unlocked:
 	if (!ret)
 		ret = -EFAULT;
 	if (ret > 0)
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v7 06/17] media: videobuf2: Move frame_vector into media subsystem
  2020-11-27 16:41 [PATCH v7 00/17] follow_pfn and other iomap races Daniel Vetter
                   ` (4 preceding siblings ...)
  2020-11-27 16:41 ` [PATCH v7 05/17] mm/frame-vector: Use FOLL_LONGTERM Daniel Vetter
@ 2020-11-27 16:41 ` Daniel Vetter
  2020-11-27 16:41 ` [PATCH v7 07/17] mm: Close race in generic_access_phys Daniel Vetter
                   ` (11 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Daniel Vetter @ 2020-11-27 16:41 UTC (permalink / raw)
  To: DRI Development, LKML
  Cc: kvm, linux-mm, linux-arm-kernel, linux-samsung-soc, linux-media,
	Daniel Vetter, John Hubbard, Hans Verkuil, Mauro Carvalho Chehab,
	Tomasz Figa, Daniel Vetter, Jason Gunthorpe, Pawel Osciak,
	Marek Szyprowski, Kyungmin Park, Mauro Carvalho Chehab,
	Andrew Morton, Jérôme Glisse, Jan Kara, Dan Williams

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).

Reviewed-by: John Hubbard <jhubbard@nvidia.com>
Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Acked-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Acked-by: Tomasz Figa <tfiga@chromium.org>
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>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
--
v3:
- Create a new frame_vector.h header for this (Mauro)
v5:
- Rebase over changes in frame-vector.c from Tomasz review.
---
 drivers/media/common/videobuf2/Kconfig        |  1 -
 drivers/media/common/videobuf2/Makefile       |  1 +
 .../media/common/videobuf2}/frame_vector.c    |  2 +
 drivers/media/platform/omap/Kconfig           |  1 -
 include/linux/mm.h                            | 42 -----------------
 include/media/frame_vector.h                  | 47 +++++++++++++++++++
 include/media/videobuf2-core.h                |  1 +
 mm/Kconfig                                    |  3 --
 mm/Makefile                                   |  1 -
 9 files changed, 51 insertions(+), 48 deletions(-)
 rename {mm => drivers/media/common/videobuf2}/frame_vector.c (99%)
 create mode 100644 include/media/frame_vector.h

diff --git a/drivers/media/common/videobuf2/Kconfig b/drivers/media/common/videobuf2/Kconfig
index edbc99ebba87..d2223a12c95f 100644
--- a/drivers/media/common/videobuf2/Kconfig
+++ b/drivers/media/common/videobuf2/Kconfig
@@ -9,7 +9,6 @@ config VIDEOBUF2_V4L2
 
 config VIDEOBUF2_MEMOPS
 	tristate
-	select FRAME_VECTOR
 
 config VIDEOBUF2_DMA_CONTIG
 	tristate
diff --git a/drivers/media/common/videobuf2/Makefile b/drivers/media/common/videobuf2/Makefile
index 77bebe8b202f..54306f8d096c 100644
--- a/drivers/media/common/videobuf2/Makefile
+++ b/drivers/media/common/videobuf2/Makefile
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 videobuf2-common-objs := videobuf2-core.o
+videobuf2-common-objs += frame_vector.o
 
 ifeq ($(CONFIG_TRACEPOINTS),y)
   videobuf2-common-objs += vb2-trace.o
diff --git a/mm/frame_vector.c b/drivers/media/common/videobuf2/frame_vector.c
similarity index 99%
rename from mm/frame_vector.c
rename to drivers/media/common/videobuf2/frame_vector.c
index f8c34b895c76..a0e65481a201 100644
--- a/mm/frame_vector.c
+++ b/drivers/media/common/videobuf2/frame_vector.c
@@ -8,6 +8,8 @@
 #include <linux/pagemap.h>
 #include <linux/sched.h>
 
+#include <media/frame_vector.h>
+
 /**
  * get_vaddr_frames() - map virtual addresses to pfns
  * @start:	starting user address
diff --git a/drivers/media/platform/omap/Kconfig b/drivers/media/platform/omap/Kconfig
index f73b5893220d..de16de46c0f4 100644
--- a/drivers/media/platform/omap/Kconfig
+++ b/drivers/media/platform/omap/Kconfig
@@ -12,6 +12,5 @@ config VIDEO_OMAP2_VOUT
 	depends on VIDEO_V4L2
 	select VIDEOBUF2_DMA_CONTIG
 	select OMAP2_VRFB if ARCH_OMAP2 || ARCH_OMAP3
-	select FRAME_VECTOR
 	help
 	  V4L2 Display driver support for OMAP2/3 based boards.
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 29a1941cd255..c4cc8ea1402c 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1751,48 +1751,6 @@ int account_locked_vm(struct mm_struct *mm, unsigned long pages, bool inc);
 int __account_locked_vm(struct mm_struct *mm, unsigned long pages, bool inc,
 			struct task_struct *task, bool bypass_rlim);
 
-/* Container for pinned pfns / pages */
-struct frame_vector {
-	unsigned int nr_allocated;	/* Number of frames we have space for */
-	unsigned int nr_frames;	/* Number of frames stored in ptrs array */
-	bool got_ref;		/* Did we pin pages by getting page ref? */
-	bool is_pfns;		/* Does array contain pages or pfns? */
-	void *ptrs[];		/* Array of pinned pfns / pages. Use
-				 * pfns_vector_pages() or pfns_vector_pfns()
-				 * for access */
-};
-
-struct frame_vector *frame_vector_create(unsigned int nr_frames);
-void frame_vector_destroy(struct frame_vector *vec);
-int get_vaddr_frames(unsigned long start, unsigned int nr_pfns,
-		     struct frame_vector *vec);
-void put_vaddr_frames(struct frame_vector *vec);
-int frame_vector_to_pages(struct frame_vector *vec);
-void frame_vector_to_pfns(struct frame_vector *vec);
-
-static inline unsigned int frame_vector_count(struct frame_vector *vec)
-{
-	return vec->nr_frames;
-}
-
-static inline struct page **frame_vector_pages(struct frame_vector *vec)
-{
-	if (vec->is_pfns) {
-		int err = frame_vector_to_pages(vec);
-
-		if (err)
-			return ERR_PTR(err);
-	}
-	return (struct page **)(vec->ptrs);
-}
-
-static inline unsigned long *frame_vector_pfns(struct frame_vector *vec)
-{
-	if (!vec->is_pfns)
-		frame_vector_to_pfns(vec);
-	return (unsigned long *)(vec->ptrs);
-}
-
 struct kvec;
 int get_kernel_pages(const struct kvec *iov, int nr_pages, int write,
 			struct page **pages);
diff --git a/include/media/frame_vector.h b/include/media/frame_vector.h
new file mode 100644
index 000000000000..bfed1710dc24
--- /dev/null
+++ b/include/media/frame_vector.h
@@ -0,0 +1,47 @@
+// SPDX-License-Identifier: GPL-2.0
+#ifndef _MEDIA_FRAME_VECTOR_H
+#define _MEDIA_FRAME_VECTOR_H
+
+/* Container for pinned pfns / pages in frame_vector.c */
+struct frame_vector {
+	unsigned int nr_allocated;	/* Number of frames we have space for */
+	unsigned int nr_frames;	/* Number of frames stored in ptrs array */
+	bool got_ref;		/* Did we pin pages by getting page ref? */
+	bool is_pfns;		/* Does array contain pages or pfns? */
+	void *ptrs[];		/* Array of pinned pfns / pages. Use
+				 * pfns_vector_pages() or pfns_vector_pfns()
+				 * for access */
+};
+
+struct frame_vector *frame_vector_create(unsigned int nr_frames);
+void frame_vector_destroy(struct frame_vector *vec);
+int get_vaddr_frames(unsigned long start, unsigned int nr_pfns,
+		     struct frame_vector *vec);
+void put_vaddr_frames(struct frame_vector *vec);
+int frame_vector_to_pages(struct frame_vector *vec);
+void frame_vector_to_pfns(struct frame_vector *vec);
+
+static inline unsigned int frame_vector_count(struct frame_vector *vec)
+{
+	return vec->nr_frames;
+}
+
+static inline struct page **frame_vector_pages(struct frame_vector *vec)
+{
+	if (vec->is_pfns) {
+		int err = frame_vector_to_pages(vec);
+
+		if (err)
+			return ERR_PTR(err);
+	}
+	return (struct page **)(vec->ptrs);
+}
+
+static inline unsigned long *frame_vector_pfns(struct frame_vector *vec)
+{
+	if (!vec->is_pfns)
+		frame_vector_to_pfns(vec);
+	return (unsigned long *)(vec->ptrs);
+}
+
+#endif /* _MEDIA_FRAME_VECTOR_H */
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index bbb3f26fbde9..d045e3a5a1d8 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -18,6 +18,7 @@
 #include <linux/dma-buf.h>
 #include <linux/bitops.h>
 #include <media/media-request.h>
+#include <media/frame_vector.h>
 
 #define VB2_MAX_FRAME	(32)
 #define VB2_MAX_PLANES	(8)
diff --git a/mm/Kconfig b/mm/Kconfig
index d42423f884a7..0dcff24cba53 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -819,9 +819,6 @@ config DEVICE_PRIVATE
 config VMAP_PFN
 	bool
 
-config FRAME_VECTOR
-	bool
-
 config ARCH_USES_HIGH_VMA_FLAGS
 	bool
 config ARCH_HAS_PKEYS
diff --git a/mm/Makefile b/mm/Makefile
index d73aed0fc99c..db41fff05038 100644
--- a/mm/Makefile
+++ b/mm/Makefile
@@ -110,7 +110,6 @@ obj-$(CONFIG_PAGE_EXTENSION) += page_ext.o
 obj-$(CONFIG_CMA_DEBUGFS) += cma_debug.o
 obj-$(CONFIG_USERFAULTFD) += userfaultfd.o
 obj-$(CONFIG_IDLE_PAGE_TRACKING) += page_idle.o
-obj-$(CONFIG_FRAME_VECTOR) += frame_vector.o
 obj-$(CONFIG_DEBUG_PAGE_REF) += debug_page_ref.o
 obj-$(CONFIG_HARDENED_USERCOPY) += usercopy.o
 obj-$(CONFIG_PERCPU_STATS) += percpu-stats.o
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v7 07/17] mm: Close race in generic_access_phys
  2020-11-27 16:41 [PATCH v7 00/17] follow_pfn and other iomap races Daniel Vetter
                   ` (5 preceding siblings ...)
  2020-11-27 16:41 ` [PATCH v7 06/17] media: videobuf2: Move frame_vector into media subsystem Daniel Vetter
@ 2020-11-27 16:41 ` Daniel Vetter
  2020-11-27 16:41 ` [PATCH v7 08/17] PCI: Obey iomem restrictions for procfs mmap Daniel Vetter
                   ` (10 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Daniel Vetter @ 2020-11-27 16:41 UTC (permalink / raw)
  To: DRI Development, LKML
  Cc: kvm, linux-mm, linux-arm-kernel, linux-samsung-soc, linux-media,
	Daniel Vetter, Daniel Vetter, Jason Gunthorpe, Dan Williams,
	Kees Cook, Benjamin Herrensmidt, Dave Airlie, Andrew Morton,
	John Hubbard, Jérôme Glisse, Jan Kara, Chris Wilson

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 ioremap might need to manipulate pagetables too we need to drop
the pt lock and have a retry loop if we raced.

While at it, also add kerneldoc and improve the comment for the
vma_ops->access function. It's for accessing, not for moving the
memory from iomem to system memory, as the old comment seemed to
suggest.

References: 28b2ee20c7cb ("access_process_vm device memory infrastructure")
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: Benjamin Herrensmidt <benh@kernel.crashing.org>
Cc: Dave Airlie <airlied@linux.ie>
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: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
--
v2: Fix inversion in the retry check (John).

v4: While at it, use offset_in_page (Chris Wilson)
---
 include/linux/mm.h |  3 ++-
 mm/memory.c        | 46 +++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index c4cc8ea1402c..b50cbb33d0f3 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -574,7 +574,8 @@ struct vm_operations_struct {
 	vm_fault_t (*pfn_mkwrite)(struct vm_fault *vmf);
 
 	/* called by access_process_vm when get_user_pages() fails, typically
-	 * for use by special VMAs that can switch between memory and hardware
+	 * for use by special VMAs. See also generic_access_phys() for a generic
+	 * implementation useful for any iomem mapping.
 	 */
 	int (*access)(struct vm_area_struct *vma, unsigned long addr,
 		      void *buf, int len, int write);
diff --git a/mm/memory.c b/mm/memory.c
index c48f8df6e502..ac32039ce941 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4847,28 +4847,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;
-	int offset = addr & (PAGE_SIZE-1);
+	pte_t *ptep, pte;
+	spinlock_t *ptl;
+	int offset = offset_in_page(addr);
+	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)) {
+		pte_unmap_unlock(ptep, ptl);
+		iounmap(maddr);
+
+		goto retry;
+	}
+
 	if (write)
 		memcpy_toio(maddr + offset, buf, len);
 	else
 		memcpy_fromio(buf, maddr + offset, len);
+	ret = len;
+	pte_unmap_unlock(ptep, ptl);
+out_unmap:
 	iounmap(maddr);
 
-	return len;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(generic_access_phys);
 #endif
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v7 08/17] PCI: Obey iomem restrictions for procfs mmap
  2020-11-27 16:41 [PATCH v7 00/17] follow_pfn and other iomap races Daniel Vetter
                   ` (6 preceding siblings ...)
  2020-11-27 16:41 ` [PATCH v7 07/17] mm: Close race in generic_access_phys Daniel Vetter
@ 2020-11-27 16:41 ` Daniel Vetter
  2020-11-27 16:41 ` [PATCH v7 09/17] /dev/mem: Only set filp->f_mapping Daniel Vetter
                   ` (9 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Daniel Vetter @ 2020-11-27 16:41 UTC (permalink / raw)
  To: DRI Development, LKML
  Cc: kvm, linux-mm, linux-arm-kernel, linux-samsung-soc, linux-media,
	Daniel Vetter, Bjorn Helgaas, Daniel Vetter, Jason Gunthorpe,
	Kees Cook, Dan Williams, Andrew Morton, John Hubbard,
	Jérôme Glisse, Jan Kara, linux-pci

There's three ways to access PCI BARs from userspace: /dev/mem, sysfs
files, and the old proc interface. Two check against
iomem_is_exclusive, proc never did. And with CONFIG_IO_STRICT_DEVMEM,
this starts to matter, since we don't want random userspace having
access to PCI BARs while a driver is loaded and using it.

Fix this by adding the same iomem_is_exclusive() check we already have
on the sysfs side in pci_mmap_resource().

Acked-by: Bjorn Helgaas <bhelgaas@google.com>
References: 90a545e98126 ("restrict /dev/mem to idle io memory ranges")
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: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
--
v2: Improve commit message (Bjorn)
---
 drivers/pci/proc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
index d35186b01d98..3a2f90beb4cb 100644
--- a/drivers/pci/proc.c
+++ b/drivers/pci/proc.c
@@ -274,6 +274,11 @@ static int proc_bus_pci_mmap(struct file *file, struct vm_area_struct *vma)
 		else
 			return -EINVAL;
 	}
+
+	if (dev->resource[i].flags & IORESOURCE_MEM &&
+	    iomem_is_exclusive(dev->resource[i].start))
+		return -EINVAL;
+
 	ret = pci_mmap_page_range(dev, i, vma,
 				  fpriv->mmap_state, write_combine);
 	if (ret < 0)
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v7 09/17] /dev/mem: Only set filp->f_mapping
  2020-11-27 16:41 [PATCH v7 00/17] follow_pfn and other iomap races Daniel Vetter
                   ` (7 preceding siblings ...)
  2020-11-27 16:41 ` [PATCH v7 08/17] PCI: Obey iomem restrictions for procfs mmap Daniel Vetter
@ 2020-11-27 16:41 ` Daniel Vetter
  2020-11-27 16:41 ` [PATCH v7 10/17] resource: Move devmem revoke code to resource framework Daniel Vetter
                   ` (8 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Daniel Vetter @ 2020-11-27 16:41 UTC (permalink / raw)
  To: DRI Development, LKML
  Cc: kvm, linux-mm, linux-arm-kernel, linux-samsung-soc, linux-media,
	Daniel Vetter, Daniel Vetter, Jason Gunthorpe, Kees Cook,
	Dan Williams, Andrew Morton, John Hubbard,
	Jérôme Glisse, Jan Kara

When we care about pagecache maintenance, we need to make sure that
both f_mapping and i_mapping point at the right mapping.

But for iomem mappings we only care about the virtual/pte side of
things, so f_mapping is enough. Also setting inode->i_mapping was
confusing me as a driver maintainer, since in e.g. drivers/gpu we
don't do that. Per Dan this seems to be copypasta from places which do
care about pagecache consistency, but not needed. Hence remove it for
slightly less confusion.

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
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/char/mem.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 94c2b556cf97..7dcf9e4ea79d 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -891,7 +891,6 @@ static int open_port(struct inode *inode, struct file *filp)
 	 * revocations when drivers want to take over a /dev/mem mapped
 	 * range.
 	 */
-	inode->i_mapping = devmem_inode->i_mapping;
 	filp->f_mapping = inode->i_mapping;
 
 	return 0;
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v7 10/17] resource: Move devmem revoke code to resource framework
  2020-11-27 16:41 [PATCH v7 00/17] follow_pfn and other iomap races Daniel Vetter
                   ` (8 preceding siblings ...)
  2020-11-27 16:41 ` [PATCH v7 09/17] /dev/mem: Only set filp->f_mapping Daniel Vetter
@ 2020-11-27 16:41 ` Daniel Vetter
  2020-11-27 16:41 ` [PATCH v7 11/17] sysfs: Support zapping of binary attr mmaps Daniel Vetter
                   ` (7 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Daniel Vetter @ 2020-11-27 16:41 UTC (permalink / raw)
  To: DRI Development, LKML
  Cc: kvm, linux-mm, linux-arm-kernel, linux-samsung-soc, linux-media,
	Daniel Vetter, Greg Kroah-Hartman, Daniel Vetter,
	Jason Gunthorpe, Kees Cook, Dan Williams, Andrew Morton,
	John Hubbard, Jérôme Glisse, Jan Kara, Arnd Bergmann,
	David Hildenbrand, Rafael J. Wysocki

We want all iomem mmaps to consistently revoke ptes when the kernel
takes over and CONFIG_IO_STRICT_DEVMEM is enabled. This includes the
pci bar mmaps available through procfs and sysfs, which currently do
not revoke mappings.

To prepare for this, move the code from the /dev/kmem driver to
kernel/resource.c.

During review Jason spotted that barriers are used somewhat
inconsistently. Fix that up while we shuffle this code, since it
doesn't have an actual impact at runtime. Otherwise no semantic and
behavioural changes intended, just code extraction and adjusting
comments and names.

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
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: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: David Hildenbrand <david@redhat.com>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
--
v3:
- add barrier for consistency and document why we don't have to check
  for NULL (Jason)
v4
- Adjust comments to reflect the general nature of this iomem revoke
  code now (Dan)
v6:
- An unintentional change which caused side-effects of this
  refactoring is that iomem_get_mapping returned NULL for
  !CONFIG_IO_STRICT_DEVMEM. But that makes core vfs code blow up,
  which assumes a mapping always exists. Undo that accidentional
  change. Reported by John.
- Augment the commit message to explain what actually changed with
  this extraction (also John).
---
 drivers/char/mem.c     | 85 +-----------------------------------
 include/linux/ioport.h |  6 +--
 kernel/resource.c      | 98 +++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 99 insertions(+), 90 deletions(-)

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 7dcf9e4ea79d..43c871dc7477 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -31,9 +31,6 @@
 #include <linux/uio.h>
 #include <linux/uaccess.h>
 #include <linux/security.h>
-#include <linux/pseudo_fs.h>
-#include <uapi/linux/magic.h>
-#include <linux/mount.h>
 
 #ifdef CONFIG_IA64
 # include <linux/efi.h>
@@ -836,42 +833,6 @@ static loff_t memory_lseek(struct file *file, loff_t offset, int orig)
 	return ret;
 }
 
-static struct inode *devmem_inode;
-
-#ifdef CONFIG_IO_STRICT_DEVMEM
-void revoke_devmem(struct resource *res)
-{
-	/* pairs with smp_store_release() in devmem_init_inode() */
-	struct inode *inode = smp_load_acquire(&devmem_inode);
-
-	/*
-	 * Check that the initialization has completed. Losing the race
-	 * is ok because it means drivers are claiming resources before
-	 * the fs_initcall level of init and prevent /dev/mem from
-	 * establishing mappings.
-	 */
-	if (!inode)
-		return;
-
-	/*
-	 * The expectation is that the driver has successfully marked
-	 * the resource busy by this point, so devmem_is_allowed()
-	 * should start returning false, however for performance this
-	 * does not iterate the entire resource range.
-	 */
-	if (devmem_is_allowed(PHYS_PFN(res->start)) &&
-	    devmem_is_allowed(PHYS_PFN(res->end))) {
-		/*
-		 * *cringe* iomem=relaxed says "go ahead, what's the
-		 * worst that can happen?"
-		 */
-		return;
-	}
-
-	unmap_mapping_range(inode->i_mapping, res->start, resource_size(res), 1);
-}
-#endif
-
 static int open_port(struct inode *inode, struct file *filp)
 {
 	int rc;
@@ -891,7 +852,7 @@ static int open_port(struct inode *inode, struct file *filp)
 	 * revocations when drivers want to take over a /dev/mem mapped
 	 * range.
 	 */
-	filp->f_mapping = inode->i_mapping;
+	filp->f_mapping = iomem_get_mapping();
 
 	return 0;
 }
@@ -1023,48 +984,6 @@ static char *mem_devnode(struct device *dev, umode_t *mode)
 
 static struct class *mem_class;
 
-static int devmem_fs_init_fs_context(struct fs_context *fc)
-{
-	return init_pseudo(fc, DEVMEM_MAGIC) ? 0 : -ENOMEM;
-}
-
-static struct file_system_type devmem_fs_type = {
-	.name		= "devmem",
-	.owner		= THIS_MODULE,
-	.init_fs_context = devmem_fs_init_fs_context,
-	.kill_sb	= kill_anon_super,
-};
-
-static int devmem_init_inode(void)
-{
-	static struct vfsmount *devmem_vfs_mount;
-	static int devmem_fs_cnt;
-	struct inode *inode;
-	int rc;
-
-	rc = simple_pin_fs(&devmem_fs_type, &devmem_vfs_mount, &devmem_fs_cnt);
-	if (rc < 0) {
-		pr_err("Cannot mount /dev/mem pseudo filesystem: %d\n", rc);
-		return rc;
-	}
-
-	inode = alloc_anon_inode(devmem_vfs_mount->mnt_sb);
-	if (IS_ERR(inode)) {
-		rc = PTR_ERR(inode);
-		pr_err("Cannot allocate inode for /dev/mem: %d\n", rc);
-		simple_release_fs(&devmem_vfs_mount, &devmem_fs_cnt);
-		return rc;
-	}
-
-	/*
-	 * Publish /dev/mem initialized.
-	 * Pairs with smp_load_acquire() in revoke_devmem().
-	 */
-	smp_store_release(&devmem_inode, inode);
-
-	return 0;
-}
-
 static int __init chr_dev_init(void)
 {
 	int minor;
@@ -1086,8 +1005,6 @@ static int __init chr_dev_init(void)
 		 */
 		if ((minor == DEVPORT_MINOR) && !arch_has_dev_port())
 			continue;
-		if ((minor == DEVMEM_MINOR) && devmem_init_inode() != 0)
-			continue;
 
 		device_create(mem_class, NULL, MKDEV(MEM_MAJOR, minor),
 			      NULL, devlist[minor].name);
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 5135d4b86cd6..02a5466245c0 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -307,11 +307,7 @@ struct resource *devm_request_free_mem_region(struct device *dev,
 struct resource *request_free_mem_region(struct resource *base,
 		unsigned long size, const char *name);
 
-#ifdef CONFIG_IO_STRICT_DEVMEM
-void revoke_devmem(struct resource *res);
-#else
-static inline void revoke_devmem(struct resource *res) { };
-#endif
+extern struct address_space *iomem_get_mapping(void);
 
 #endif /* __ASSEMBLY__ */
 #endif	/* _LINUX_IOPORT_H */
diff --git a/kernel/resource.c b/kernel/resource.c
index 3ae2f56cc79d..c3d71db7a40e 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -18,12 +18,15 @@
 #include <linux/spinlock.h>
 #include <linux/fs.h>
 #include <linux/proc_fs.h>
+#include <linux/pseudo_fs.h>
 #include <linux/sched.h>
 #include <linux/seq_file.h>
 #include <linux/device.h>
 #include <linux/pfn.h>
 #include <linux/mm.h>
+#include <linux/mount.h>
 #include <linux/resource_ext.h>
+#include <uapi/linux/magic.h>
 #include <asm/io.h>
 
 
@@ -1115,6 +1118,55 @@ resource_size_t resource_alignment(struct resource *res)
 
 static DECLARE_WAIT_QUEUE_HEAD(muxed_resource_wait);
 
+static struct inode *iomem_inode;
+
+#ifdef CONFIG_IO_STRICT_DEVMEM
+static void revoke_iomem(struct resource *res)
+{
+	/* pairs with smp_store_release() in iomem_init_inode() */
+	struct inode *inode = smp_load_acquire(&iomem_inode);
+
+	/*
+	 * Check that the initialization has completed. Losing the race
+	 * is ok because it means drivers are claiming resources before
+	 * the fs_initcall level of init and prevent iomem_get_mapping users
+	 * from establishing mappings.
+	 */
+	if (!inode)
+		return;
+
+	/*
+	 * The expectation is that the driver has successfully marked
+	 * the resource busy by this point, so devmem_is_allowed()
+	 * should start returning false, however for performance this
+	 * does not iterate the entire resource range.
+	 */
+	if (devmem_is_allowed(PHYS_PFN(res->start)) &&
+	    devmem_is_allowed(PHYS_PFN(res->end))) {
+		/*
+		 * *cringe* iomem=relaxed says "go ahead, what's the
+		 * worst that can happen?"
+		 */
+		return;
+	}
+
+	unmap_mapping_range(inode->i_mapping, res->start, resource_size(res), 1);
+}
+#else
+static void revoke_iomem(struct resource *res) {}
+#endif
+
+struct address_space *iomem_get_mapping(void)
+{
+	/*
+	 * This function is only called from file open paths, hence guaranteed
+	 * that fs_initcalls have completed and no need to check for NULL. But
+	 * since revoke_iomem can be called before the initcall we still need
+	 * the barrier to appease checkers.
+	 */
+	return smp_load_acquire(&iomem_inode)->i_mapping;
+}
+
 /**
  * __request_region - create a new busy resource region
  * @parent: parent resource descriptor
@@ -1182,7 +1234,7 @@ struct resource * __request_region(struct resource *parent,
 	write_unlock(&resource_lock);
 
 	if (res && orig_parent == &iomem_resource)
-		revoke_devmem(res);
+		revoke_iomem(res);
 
 	return res;
 }
@@ -1782,4 +1834,48 @@ static int __init strict_iomem(char *str)
 	return 1;
 }
 
+static int iomem_fs_init_fs_context(struct fs_context *fc)
+{
+	return init_pseudo(fc, DEVMEM_MAGIC) ? 0 : -ENOMEM;
+}
+
+static struct file_system_type iomem_fs_type = {
+	.name		= "iomem",
+	.owner		= THIS_MODULE,
+	.init_fs_context = iomem_fs_init_fs_context,
+	.kill_sb	= kill_anon_super,
+};
+
+static int __init iomem_init_inode(void)
+{
+	static struct vfsmount *iomem_vfs_mount;
+	static int iomem_fs_cnt;
+	struct inode *inode;
+	int rc;
+
+	rc = simple_pin_fs(&iomem_fs_type, &iomem_vfs_mount, &iomem_fs_cnt);
+	if (rc < 0) {
+		pr_err("Cannot mount iomem pseudo filesystem: %d\n", rc);
+		return rc;
+	}
+
+	inode = alloc_anon_inode(iomem_vfs_mount->mnt_sb);
+	if (IS_ERR(inode)) {
+		rc = PTR_ERR(inode);
+		pr_err("Cannot allocate inode for iomem: %d\n", rc);
+		simple_release_fs(&iomem_vfs_mount, &iomem_fs_cnt);
+		return rc;
+	}
+
+	/*
+	 * Publish iomem revocation inode initialized.
+	 * Pairs with smp_load_acquire() in revoke_iomem().
+	 */
+	smp_store_release(&iomem_inode, inode);
+
+	return 0;
+}
+
+fs_initcall(iomem_init_inode);
+
 __setup("iomem=", strict_iomem);
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v7 11/17] sysfs: Support zapping of binary attr mmaps
  2020-11-27 16:41 [PATCH v7 00/17] follow_pfn and other iomap races Daniel Vetter
                   ` (9 preceding siblings ...)
  2020-11-27 16:41 ` [PATCH v7 10/17] resource: Move devmem revoke code to resource framework Daniel Vetter
@ 2020-11-27 16:41 ` Daniel Vetter
  2020-11-27 16:41 ` [PATCH v7 12/17] PCI: Revoke mappings like devmem Daniel Vetter
                   ` (6 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Daniel Vetter @ 2020-11-27 16:41 UTC (permalink / raw)
  To: DRI Development, LKML
  Cc: kvm, linux-mm, linux-arm-kernel, linux-samsung-soc, linux-media,
	Daniel Vetter, Greg Kroah-Hartman, Daniel Vetter,
	Jason Gunthorpe, Kees Cook, Dan Williams, Andrew Morton,
	John Hubbard, Jérôme Glisse, Jan Kara, Bjorn Helgaas,
	linux-pci, Rafael J. Wysocki, Christian Brauner, David S. Miller,
	Michael Ellerman, Sourabh Jain, Mauro Carvalho Chehab,
	Nayna Jain

We want to be able to revoke pci mmaps so that the same access rules
applies as for /dev/kmem. Revoke support for devmem was added in
3234ac664a87 ("/dev/mem: Revoke mappings when a driver claims the
region").

The simplest way to achieve this is by having the same filp->f_mapping
for all mappings, so that unmap_mapping_range can find them all, no
matter through which file they've been created. Since this must be set
at open time we need sysfs support for this.

Add an optional mapping parameter bin_attr, which is only consulted
when there's also an mmap callback, since without mmap support
allowing to adjust the ->f_mapping makes no sense.

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
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: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Christian Brauner <christian.brauner@ubuntu.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Sourabh Jain <sourabhjain@linux.ibm.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Cc: Nayna Jain <nayna@linux.ibm.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 fs/sysfs/file.c       | 11 +++++++++++
 include/linux/sysfs.h |  2 ++
 2 files changed, 13 insertions(+)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 96d0da65e088..9aefa7779b29 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -170,6 +170,16 @@ static int sysfs_kf_bin_mmap(struct kernfs_open_file *of,
 	return battr->mmap(of->file, kobj, battr, vma);
 }
 
+static int sysfs_kf_bin_open(struct kernfs_open_file *of)
+{
+	struct bin_attribute *battr = of->kn->priv;
+
+	if (battr->mapping)
+		of->file->f_mapping = battr->mapping;
+
+	return 0;
+}
+
 void sysfs_notify(struct kobject *kobj, const char *dir, const char *attr)
 {
 	struct kernfs_node *kn = kobj->sd, *tmp;
@@ -241,6 +251,7 @@ static const struct kernfs_ops sysfs_bin_kfops_mmap = {
 	.read		= sysfs_kf_bin_read,
 	.write		= sysfs_kf_bin_write,
 	.mmap		= sysfs_kf_bin_mmap,
+	.open		= sysfs_kf_bin_open,
 };
 
 int sysfs_add_file_mode_ns(struct kernfs_node *parent,
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 2caa34c1ca1a..d76a1ddf83a3 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -164,11 +164,13 @@ __ATTRIBUTE_GROUPS(_name)
 
 struct file;
 struct vm_area_struct;
+struct address_space;
 
 struct bin_attribute {
 	struct attribute	attr;
 	size_t			size;
 	void			*private;
+	struct address_space	*mapping;
 	ssize_t (*read)(struct file *, struct kobject *, struct bin_attribute *,
 			char *, loff_t, size_t);
 	ssize_t (*write)(struct file *, struct kobject *, struct bin_attribute *,
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v7 12/17] PCI: Revoke mappings like devmem
  2020-11-27 16:41 [PATCH v7 00/17] follow_pfn and other iomap races Daniel Vetter
                   ` (10 preceding siblings ...)
  2020-11-27 16:41 ` [PATCH v7 11/17] sysfs: Support zapping of binary attr mmaps Daniel Vetter
@ 2020-11-27 16:41 ` Daniel Vetter
  2021-01-19  8:17   ` Daniel Vetter
  2020-11-27 16:41 ` [PATCH v7 13/17] mm: Add unsafe_follow_pfn Daniel Vetter
                   ` (5 subsequent siblings)
  17 siblings, 1 reply; 32+ messages in thread
From: Daniel Vetter @ 2020-11-27 16:41 UTC (permalink / raw)
  To: DRI Development, LKML
  Cc: kvm, linux-mm, linux-arm-kernel, linux-samsung-soc, linux-media,
	Daniel Vetter, Bjorn Helgaas, Dan Williams, Daniel Vetter,
	Jason Gunthorpe, Kees Cook, Andrew Morton, John Hubbard,
	Jérôme Glisse, Jan Kara, Greg Kroah-Hartman, linux-pci

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.

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. The cleanest way is
to adjust this at at ->open time:

- for sysfs this is easy, now that binary attributes support this. We
  just set bin_attr->mapping when mmap is supported
- for procfs it's a bit more tricky, since procfs pci access has only
  one file per device, and access to a specific resources first needs
  to be set up with some ioctl calls. But mmap is only supported for
  the same resources as sysfs exposes with mmap support, and otherwise
  rejected, so we can set the mapping unconditionally at open time
  without harm.

A special consideration is for arch_can_pci_mmap_io() - we need to
make sure that the ->f_mapping doesn't alias between ioport and iomem
space. There's only 2 ways in-tree to support mmap of ioports: generic
pci mmap (ARCH_GENERIC_PCI_MMAP_RESOURCE), and sparc as the single
architecture hand-rolling. Both approach support ioport mmap through a
special pfn range and not through magic pte attributes. Aliasing is
therefore not a problem.

The only difference in access checks left is that sysfs PCI mmap does
not check for CAP_RAWIO. I'm not really sure whether that should be
added or not.

Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
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: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
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: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
--
v2:
- Totally new approach: Adjust filp->f_mapping at open time. Note that
  this now works on all architectures, not just those support
  ARCH_GENERIC_PCI_MMAP_RESOURCE
---
 drivers/pci/pci-sysfs.c | 4 ++++
 drivers/pci/proc.c      | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index d15c881e2e7e..3f1c31bc0b7c 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -929,6 +929,7 @@ void pci_create_legacy_files(struct pci_bus *b)
 	b->legacy_io->read = pci_read_legacy_io;
 	b->legacy_io->write = pci_write_legacy_io;
 	b->legacy_io->mmap = pci_mmap_legacy_io;
+	b->legacy_io->mapping = iomem_get_mapping();
 	pci_adjust_legacy_attr(b, pci_mmap_io);
 	error = device_create_bin_file(&b->dev, b->legacy_io);
 	if (error)
@@ -941,6 +942,7 @@ void pci_create_legacy_files(struct pci_bus *b)
 	b->legacy_mem->size = 1024*1024;
 	b->legacy_mem->attr.mode = 0600;
 	b->legacy_mem->mmap = pci_mmap_legacy_mem;
+	b->legacy_io->mapping = iomem_get_mapping();
 	pci_adjust_legacy_attr(b, pci_mmap_mem);
 	error = device_create_bin_file(&b->dev, b->legacy_mem);
 	if (error)
@@ -1156,6 +1158,8 @@ static int pci_create_attr(struct pci_dev *pdev, int num, int write_combine)
 			res_attr->mmap = pci_mmap_resource_uc;
 		}
 	}
+	if (res_attr->mmap)
+		res_attr->mapping = iomem_get_mapping();
 	res_attr->attr.name = res_attr_name;
 	res_attr->attr.mode = 0600;
 	res_attr->size = pci_resource_len(pdev, num);
diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
index 3a2f90beb4cb..9bab07302bbf 100644
--- a/drivers/pci/proc.c
+++ b/drivers/pci/proc.c
@@ -298,6 +298,7 @@ static int proc_bus_pci_open(struct inode *inode, struct file *file)
 	fpriv->write_combine = 0;
 
 	file->private_data = fpriv;
+	file->f_mapping = iomem_get_mapping();
 
 	return 0;
 }
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v7 13/17] mm: Add unsafe_follow_pfn
  2020-11-27 16:41 [PATCH v7 00/17] follow_pfn and other iomap races Daniel Vetter
                   ` (11 preceding siblings ...)
  2020-11-27 16:41 ` [PATCH v7 12/17] PCI: Revoke mappings like devmem Daniel Vetter
@ 2020-11-27 16:41 ` Daniel Vetter
  2020-11-27 16:41 ` [PATCH v7 14/17] media/videobuf1|2: Mark follow_pfn usage as unsafe Daniel Vetter
                   ` (4 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Daniel Vetter @ 2020-11-27 16:41 UTC (permalink / raw)
  To: DRI Development, LKML
  Cc: kvm, linux-mm, linux-arm-kernel, linux-samsung-soc, linux-media,
	Daniel Vetter, Daniel Vetter, Christoph Hellwig, Jason Gunthorpe,
	Kees Cook, Dan Williams, Andrew Morton, John Hubbard,
	Jérôme Glisse, Jan Kara

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.

Also mark up follow_pfn as EXPORT_SYMBOL_GPL. The only safe way to use
that by drivers/modules is together with an mmu_notifier, and that's
all _GPL stuff.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Christoph Hellwig <hch@infradead.org>
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
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
--
v5: Suggestions from Christoph
- reindent for less weirdness
- use IS_ENABLED instead of #ifdef
- same checks for nommu, for consistency
- EXPORT_SYMBOL_GPL for follow_pfn.
- kerneldoc was already updated in previous versions to explain when
  follow_pfn can be used safely
---
 include/linux/mm.h |  2 ++
 mm/memory.c        | 34 ++++++++++++++++++++++++++++++++--
 mm/nommu.c         | 27 ++++++++++++++++++++++++++-
 security/Kconfig   | 13 +++++++++++++
 4 files changed, 73 insertions(+), 3 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index b50cbb33d0f3..bb3e926afd91 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1661,6 +1661,8 @@ int follow_pte_pmd(struct mm_struct *mm, unsigned long address,
 		   pte_t **ptepp, pmd_t **pmdpp, spinlock_t **ptlp);
 int follow_pfn(struct vm_area_struct *vma, unsigned long address,
 	unsigned long *pfn);
+int unsafe_follow_pfn(struct vm_area_struct *vma, unsigned long address,
+		      unsigned long *pfn);
 int follow_phys(struct vm_area_struct *vma, unsigned long address,
 		unsigned int flags, unsigned long *prot, resource_size_t *phys);
 int generic_access_phys(struct vm_area_struct *vma, unsigned long addr,
diff --git a/mm/memory.c b/mm/memory.c
index ac32039ce941..0db0c5e233fd 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4795,7 +4795,12 @@ EXPORT_SYMBOL(follow_pte_pmd);
  * @address: user virtual address
  * @pfn: location to store found PFN
  *
- * Only IO mappings and raw PFN mappings are allowed.
+ * Only IO mappings and raw PFN mappings are allowed. Note that callers must
+ * ensure coherency with pte updates by using a &mmu_notifier to follow updates.
+ * If this is not feasible, or the access to the @pfn is only very short term,
+ * use follow_pte_pmd() instead and hold the pagetable lock for the duration of
+ * the access instead. Any caller not following these requirements must use
+ * unsafe_follow_pfn() instead.
  *
  * Return: zero and the pfn at @pfn on success, -ve otherwise.
  */
@@ -4816,7 +4821,32 @@ int follow_pfn(struct vm_area_struct *vma, unsigned long address,
 	pte_unmap_unlock(ptep, ptl);
 	return 0;
 }
-EXPORT_SYMBOL(follow_pfn);
+EXPORT_SYMBOL_GPL(follow_pfn);
+
+/**
+ * unsafe_follow_pfn - look up PFN at a user virtual address
+ * @vma: memory mapping
+ * @address: user virtual address
+ * @pfn: location to store found PFN
+ *
+ * Only IO mappings and raw PFN mappings are allowed.
+ *
+ * Returns zero and the pfn at @pfn on success, -ve otherwise.
+ */
+int unsafe_follow_pfn(struct vm_area_struct *vma, unsigned long address,
+		      unsigned long *pfn)
+{
+	if (IS_ENABLED(CONFIG_STRICT_FOLLOW_PFN)) {
+		pr_info("unsafe follow_pfn usage rejected, see CONFIG_STRICT_FOLLOW_PFN\n");
+		return -EINVAL;
+	}
+
+	WARN_ONCE(1, "unsafe follow_pfn usage\n");
+	add_taint(TAINT_USER, LOCKDEP_STILL_OK);
+
+	return follow_pfn(vma, address, pfn);
+}
+EXPORT_SYMBOL(unsafe_follow_pfn);
 
 #ifdef CONFIG_HAVE_IOREMAP_PROT
 int follow_phys(struct vm_area_struct *vma,
diff --git a/mm/nommu.c b/mm/nommu.c
index 0faf39b32cdb..79fc98a6c94a 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -130,7 +130,32 @@ int follow_pfn(struct vm_area_struct *vma, unsigned long address,
 	*pfn = address >> PAGE_SHIFT;
 	return 0;
 }
-EXPORT_SYMBOL(follow_pfn);
+EXPORT_SYMBOL_GPL(follow_pfn);
+
+/**
+ * unsafe_follow_pfn - look up PFN at a user virtual address
+ * @vma: memory mapping
+ * @address: user virtual address
+ * @pfn: location to store found PFN
+ *
+ * Only IO mappings and raw PFN mappings are allowed.
+ *
+ * Returns zero and the pfn at @pfn on success, -ve otherwise.
+ */
+int unsafe_follow_pfn(struct vm_area_struct *vma, unsigned long address,
+		      unsigned long *pfn)
+{
+	if (IS_ENABLED(CONFIG_STRICT_FOLLOW_PFN)) {
+		pr_info("unsafe follow_pfn usage rejected, see CONFIG_STRICT_FOLLOW_PFN\n");
+		return -EINVAL;
+	}
+
+	WARN_ONCE(1, "unsafe follow_pfn usage\n");
+	add_taint(TAINT_USER, LOCKDEP_STILL_OK);
+
+	return follow_pfn(vma, address, pfn);
+}
+EXPORT_SYMBOL(unsafe_follow_pfn);
 
 LIST_HEAD(vmap_area_list);
 
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
+	help
+	  Some functionality in the kernel follows userspace mappings to iomem
+	  ranges in an unsafe matter. Examples include v4l userptr for zero-copy
+	  buffers sharing.
+
+	  If this option is switched on, such access is rejected. Only enable
+	  this option when you must run userspace which requires this.
+
+	  If in doubt, say Y.
+
 source "security/selinux/Kconfig"
 source "security/smack/Kconfig"
 source "security/tomoyo/Kconfig"
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v7 14/17] media/videobuf1|2: Mark follow_pfn usage as unsafe
  2020-11-27 16:41 [PATCH v7 00/17] follow_pfn and other iomap races Daniel Vetter
                   ` (12 preceding siblings ...)
  2020-11-27 16:41 ` [PATCH v7 13/17] mm: Add unsafe_follow_pfn Daniel Vetter
@ 2020-11-27 16:41 ` Daniel Vetter
  2021-01-19 12:24   ` Hans Verkuil
  2020-11-27 16:41 ` [PATCH v7 15/17] vfio/type1: Mark follow_pfn " Daniel Vetter
                   ` (3 subsequent siblings)
  17 siblings, 1 reply; 32+ messages in thread
From: Daniel Vetter @ 2020-11-27 16:41 UTC (permalink / raw)
  To: DRI Development, LKML
  Cc: kvm, linux-mm, linux-arm-kernel, linux-samsung-soc, linux-media,
	Daniel Vetter, Tomasz Figa, Daniel Vetter, Jason Gunthorpe,
	Kees Cook, Dan Williams, Andrew Morton, John Hubbard,
	Jérôme Glisse, Jan Kara, Pawel Osciak,
	Marek Szyprowski, Kyungmin Park, Laurent Dufour, Vlastimil Babka,
	Daniel Jordan, Michel Lespinasse

The media model assumes that buffers are all preallocated, so that
when a media pipeline is running we never miss a deadline because the
buffers aren't allocated or available.

This means we cannot fix the v4l follow_pfn usage through
mmu_notifier, without breaking how this all works. The only real fix
is to deprecate userptr support for VM_IO | VM_PFNMAP mappings and
tell everyone to cut over to dma-buf memory sharing for zerocopy.

userptr for normal memory will keep working as-is, this only affects
the zerocopy userptr usage enabled in 50ac952d2263 ("[media]
videobuf2-dma-sg: Support io userptr operations on io memory").

Acked-by: Tomasz Figa <tfiga@chromium.org>
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: 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: Laurent Dufour <ldufour@linux.ibm.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Michel Lespinasse <walken@google.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
--
v3:
- Reference the commit that enabled the zerocopy userptr use case to
  make it abundandtly clear that this patch only affects that, and not
  normal memory userptr. The old commit message already explained that
  normal memory userptr is unaffected, but I guess that was not clear
  enough.
---
 drivers/media/common/videobuf2/frame_vector.c | 2 +-
 drivers/media/v4l2-core/videobuf-dma-contig.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/common/videobuf2/frame_vector.c b/drivers/media/common/videobuf2/frame_vector.c
index a0e65481a201..1a82ec13ea00 100644
--- a/drivers/media/common/videobuf2/frame_vector.c
+++ b/drivers/media/common/videobuf2/frame_vector.c
@@ -70,7 +70,7 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames,
 			break;
 
 		while (ret < nr_frames && start + PAGE_SIZE <= vma->vm_end) {
-			err = follow_pfn(vma, start, &nums[ret]);
+			err = unsafe_follow_pfn(vma, start, &nums[ret]);
 			if (err) {
 				if (ret == 0)
 					ret = err;
diff --git a/drivers/media/v4l2-core/videobuf-dma-contig.c b/drivers/media/v4l2-core/videobuf-dma-contig.c
index 52312ce2ba05..821c4a76ab96 100644
--- a/drivers/media/v4l2-core/videobuf-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf-dma-contig.c
@@ -183,7 +183,7 @@ static int videobuf_dma_contig_user_get(struct videobuf_dma_contig_memory *mem,
 	user_address = untagged_baddr;
 
 	while (pages_done < (mem->size >> PAGE_SHIFT)) {
-		ret = follow_pfn(vma, user_address, &this_pfn);
+		ret = unsafe_follow_pfn(vma, user_address, &this_pfn);
 		if (ret)
 			break;
 
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v7 15/17] vfio/type1: Mark follow_pfn as unsafe
  2020-11-27 16:41 [PATCH v7 00/17] follow_pfn and other iomap races Daniel Vetter
                   ` (13 preceding siblings ...)
  2020-11-27 16:41 ` [PATCH v7 14/17] media/videobuf1|2: Mark follow_pfn usage as unsafe Daniel Vetter
@ 2020-11-27 16:41 ` Daniel Vetter
  2020-11-27 16:41 ` [PATCH v7 16/17] kvm: pass kvm argument to follow_pfn callsites Daniel Vetter
                   ` (2 subsequent siblings)
  17 siblings, 0 replies; 32+ messages in thread
From: Daniel Vetter @ 2020-11-27 16:41 UTC (permalink / raw)
  To: DRI Development, LKML
  Cc: kvm, linux-mm, linux-arm-kernel, linux-samsung-soc, linux-media,
	Daniel Vetter, Daniel Vetter, Jason Gunthorpe, Kees Cook,
	Dan Williams, Andrew Morton, John Hubbard,
	Jérôme Glisse, Jan Kara, Alex Williamson,
	Cornelia Huck

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
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 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 67e827638995..10170723bb58 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);
 	}
 
 	return ret;
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v7 16/17] kvm: pass kvm argument to follow_pfn callsites
  2020-11-27 16:41 [PATCH v7 00/17] follow_pfn and other iomap races Daniel Vetter
                   ` (14 preceding siblings ...)
  2020-11-27 16:41 ` [PATCH v7 15/17] vfio/type1: Mark follow_pfn " Daniel Vetter
@ 2020-11-27 16:41 ` Daniel Vetter
  2020-11-27 16:41 ` [PATCH v7 17/17] mm: add mmu_notifier argument to follow_pfn Daniel Vetter
  2021-01-12 13:24 ` [PATCH v7 00/17] follow_pfn and other iomap races Daniel Vetter
  17 siblings, 0 replies; 32+ messages in thread
From: Daniel Vetter @ 2020-11-27 16:41 UTC (permalink / raw)
  To: DRI Development, LKML
  Cc: kvm, linux-mm, linux-arm-kernel, linux-samsung-soc, linux-media,
	Daniel Vetter, Paolo Bonzini, Daniel Vetter, Christoph Hellwig,
	Jason Gunthorpe, Kees Cook, Dan Williams, Andrew Morton,
	John Hubbard, Jérôme Glisse, Jan Kara

Both Christoph Hellwig and Jason Gunthorpe suggested that usage of
follow_pfn by modules should be locked down more. To do so callers
need to be able to pass the mmu_notifier subscription corresponding
to the mm_struct to follow_pfn().

This patch does the rote work of doing that in the kvm subsystem. In
most places this is solved by passing struct kvm * down the call
stacks as an additional parameter, since that contains the
mmu_notifier.

Compile tested on all affected arch.

Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Christoph Hellwig <hch@infradead.org>
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
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 arch/powerpc/kvm/book3s_64_mmu_hv.c    |  2 +-
 arch/powerpc/kvm/book3s_64_mmu_radix.c |  2 +-
 arch/powerpc/kvm/e500_mmu_host.c       |  2 +-
 arch/x86/kvm/mmu/mmu.c                 |  8 ++--
 include/linux/kvm_host.h               |  9 +++--
 virt/kvm/kvm_main.c                    | 52 +++++++++++++++-----------
 6 files changed, 45 insertions(+), 30 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
index 38ea396a23d6..86781ff76fcb 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -589,7 +589,7 @@ int kvmppc_book3s_hv_page_fault(struct kvm_vcpu *vcpu,
 		write_ok = true;
 	} else {
 		/* Call KVM generic code to do the slow-path check */
-		pfn = __gfn_to_pfn_memslot(memslot, gfn, false, NULL,
+		pfn = __gfn_to_pfn_memslot(kvm, memslot, gfn, false, NULL,
 					   writing, &write_ok);
 		if (is_error_noslot_pfn(pfn))
 			return -EFAULT;
diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c
index bb35490400e9..319a1a99153f 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
@@ -821,7 +821,7 @@ int kvmppc_book3s_instantiate_page(struct kvm_vcpu *vcpu,
 		unsigned long pfn;
 
 		/* Call KVM generic code to do the slow-path check */
-		pfn = __gfn_to_pfn_memslot(memslot, gfn, false, NULL,
+		pfn = __gfn_to_pfn_memslot(kvm, memslot, gfn, false, NULL,
 					   writing, upgrade_p);
 		if (is_error_noslot_pfn(pfn))
 			return -EFAULT;
diff --git a/arch/powerpc/kvm/e500_mmu_host.c b/arch/powerpc/kvm/e500_mmu_host.c
index ed0c9c43d0cf..fd2b2d363559 100644
--- a/arch/powerpc/kvm/e500_mmu_host.c
+++ b/arch/powerpc/kvm/e500_mmu_host.c
@@ -446,7 +446,7 @@ static inline int kvmppc_e500_shadow_map(struct kvmppc_vcpu_e500 *vcpu_e500,
 
 	if (likely(!pfnmap)) {
 		tsize_pages = 1UL << (tsize + 10 - PAGE_SHIFT);
-		pfn = gfn_to_pfn_memslot(slot, gfn);
+		pfn = gfn_to_pfn_memslot(kvm, slot, gfn);
 		if (is_error_noslot_pfn(pfn)) {
 			if (printk_ratelimit())
 				pr_err("%s: real page not found for gfn %lx\n",
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 5bb1939b65d8..67ada47aa722 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -2679,7 +2679,7 @@ static kvm_pfn_t pte_prefetch_gfn_to_pfn(struct kvm_vcpu *vcpu, gfn_t gfn,
 	if (!slot)
 		return KVM_PFN_ERR_FAULT;
 
-	return gfn_to_pfn_memslot_atomic(slot, gfn);
+	return gfn_to_pfn_memslot_atomic(vcpu->kvm, slot, gfn);
 }
 
 static int direct_pte_prefetch_many(struct kvm_vcpu *vcpu,
@@ -3657,7 +3657,8 @@ static bool try_async_pf(struct kvm_vcpu *vcpu, bool prefault, gfn_t gfn,
 	}
 
 	async = false;
-	*pfn = __gfn_to_pfn_memslot(slot, gfn, false, &async, write, writable);
+	*pfn = __gfn_to_pfn_memslot(vcpu->kvm, slot, gfn,
+				    false, &async, write, writable);
 	if (!async)
 		return false; /* *pfn has correct page already */
 
@@ -3671,7 +3672,8 @@ static bool try_async_pf(struct kvm_vcpu *vcpu, bool prefault, gfn_t gfn,
 			return true;
 	}
 
-	*pfn = __gfn_to_pfn_memslot(slot, gfn, false, NULL, write, writable);
+	*pfn = __gfn_to_pfn_memslot(vcpu->kvm, slot, gfn,
+				    false, NULL, write, writable);
 	return false;
 }
 
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 7f2e2a09ebbd..864424ce6b6b 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -717,9 +717,12 @@ void kvm_set_page_accessed(struct page *page);
 kvm_pfn_t gfn_to_pfn(struct kvm *kvm, gfn_t gfn);
 kvm_pfn_t gfn_to_pfn_prot(struct kvm *kvm, gfn_t gfn, bool write_fault,
 		      bool *writable);
-kvm_pfn_t gfn_to_pfn_memslot(struct kvm_memory_slot *slot, gfn_t gfn);
-kvm_pfn_t gfn_to_pfn_memslot_atomic(struct kvm_memory_slot *slot, gfn_t gfn);
-kvm_pfn_t __gfn_to_pfn_memslot(struct kvm_memory_slot *slot, gfn_t gfn,
+kvm_pfn_t gfn_to_pfn_memslot(struct kvm *kvm,
+			     struct kvm_memory_slot *slot, gfn_t gfn);
+kvm_pfn_t gfn_to_pfn_memslot_atomic(struct kvm *kvm,
+				    struct kvm_memory_slot *slot, gfn_t gfn);
+kvm_pfn_t __gfn_to_pfn_memslot(struct kvm *kvm,
+			       struct kvm_memory_slot *slot, gfn_t gfn,
 			       bool atomic, bool *async, bool write_fault,
 			       bool *writable);
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 2541a17ff1c4..417f3d470c3e 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1883,7 +1883,7 @@ static bool vma_is_valid(struct vm_area_struct *vma, bool write_fault)
 	return true;
 }
 
-static int hva_to_pfn_remapped(struct vm_area_struct *vma,
+static int hva_to_pfn_remapped(struct kvm *kvm, struct vm_area_struct *vma,
 			       unsigned long addr, bool *async,
 			       bool write_fault, bool *writable,
 			       kvm_pfn_t *p_pfn)
@@ -1946,8 +1946,9 @@ static int hva_to_pfn_remapped(struct vm_area_struct *vma,
  * 2): @write_fault = false && @writable, @writable will tell the caller
  *     whether the mapping is writable.
  */
-static kvm_pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async,
-			bool write_fault, bool *writable)
+static kvm_pfn_t hva_to_pfn(struct kvm *kvm, unsigned long addr,
+			    bool atomic, bool *async,
+			    bool write_fault, bool *writable)
 {
 	struct vm_area_struct *vma;
 	kvm_pfn_t pfn = 0;
@@ -1979,7 +1980,8 @@ static kvm_pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async,
 	if (vma == NULL)
 		pfn = KVM_PFN_ERR_FAULT;
 	else if (vma->vm_flags & (VM_IO | VM_PFNMAP)) {
-		r = hva_to_pfn_remapped(vma, addr, async, write_fault, writable, &pfn);
+		r = hva_to_pfn_remapped(kvm, vma, addr,
+					async, write_fault, writable, &pfn);
 		if (r == -EAGAIN)
 			goto retry;
 		if (r < 0)
@@ -1994,7 +1996,8 @@ static kvm_pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async,
 	return pfn;
 }
 
-kvm_pfn_t __gfn_to_pfn_memslot(struct kvm_memory_slot *slot, gfn_t gfn,
+kvm_pfn_t __gfn_to_pfn_memslot(struct kvm *kvm,
+			       struct kvm_memory_slot *slot, gfn_t gfn,
 			       bool atomic, bool *async, bool write_fault,
 			       bool *writable)
 {
@@ -2018,7 +2021,7 @@ kvm_pfn_t __gfn_to_pfn_memslot(struct kvm_memory_slot *slot, gfn_t gfn,
 		writable = NULL;
 	}
 
-	return hva_to_pfn(addr, atomic, async, write_fault,
+	return hva_to_pfn(kvm, addr, atomic, async, write_fault,
 			  writable);
 }
 EXPORT_SYMBOL_GPL(__gfn_to_pfn_memslot);
@@ -2026,38 +2029,43 @@ EXPORT_SYMBOL_GPL(__gfn_to_pfn_memslot);
 kvm_pfn_t gfn_to_pfn_prot(struct kvm *kvm, gfn_t gfn, bool write_fault,
 		      bool *writable)
 {
-	return __gfn_to_pfn_memslot(gfn_to_memslot(kvm, gfn), gfn, false, NULL,
+	return __gfn_to_pfn_memslot(kvm, gfn_to_memslot(kvm, gfn), gfn,
+				    false, NULL,
 				    write_fault, writable);
 }
 EXPORT_SYMBOL_GPL(gfn_to_pfn_prot);
 
-kvm_pfn_t gfn_to_pfn_memslot(struct kvm_memory_slot *slot, gfn_t gfn)
+kvm_pfn_t gfn_to_pfn_memslot(struct kvm *kvm,
+			     struct kvm_memory_slot *slot, gfn_t gfn)
 {
-	return __gfn_to_pfn_memslot(slot, gfn, false, NULL, true, NULL);
+	return __gfn_to_pfn_memslot(kvm, slot, gfn, false, NULL, true, NULL);
 }
 EXPORT_SYMBOL_GPL(gfn_to_pfn_memslot);
 
-kvm_pfn_t gfn_to_pfn_memslot_atomic(struct kvm_memory_slot *slot, gfn_t gfn)
+kvm_pfn_t gfn_to_pfn_memslot_atomic(struct kvm *kvm,
+				    struct kvm_memory_slot *slot, gfn_t gfn)
 {
-	return __gfn_to_pfn_memslot(slot, gfn, true, NULL, true, NULL);
+	return __gfn_to_pfn_memslot(kvm, slot, gfn, true, NULL, true, NULL);
 }
 EXPORT_SYMBOL_GPL(gfn_to_pfn_memslot_atomic);
 
 kvm_pfn_t kvm_vcpu_gfn_to_pfn_atomic(struct kvm_vcpu *vcpu, gfn_t gfn)
 {
-	return gfn_to_pfn_memslot_atomic(kvm_vcpu_gfn_to_memslot(vcpu, gfn), gfn);
+	return gfn_to_pfn_memslot_atomic(vcpu->kvm,
+					 kvm_vcpu_gfn_to_memslot(vcpu, gfn), gfn);
 }
 EXPORT_SYMBOL_GPL(kvm_vcpu_gfn_to_pfn_atomic);
 
 kvm_pfn_t gfn_to_pfn(struct kvm *kvm, gfn_t gfn)
 {
-	return gfn_to_pfn_memslot(gfn_to_memslot(kvm, gfn), gfn);
+	return gfn_to_pfn_memslot(kvm, gfn_to_memslot(kvm, gfn), gfn);
 }
 EXPORT_SYMBOL_GPL(gfn_to_pfn);
 
 kvm_pfn_t kvm_vcpu_gfn_to_pfn(struct kvm_vcpu *vcpu, gfn_t gfn)
 {
-	return gfn_to_pfn_memslot(kvm_vcpu_gfn_to_memslot(vcpu, gfn), gfn);
+	return gfn_to_pfn_memslot(vcpu->kvm,
+				  kvm_vcpu_gfn_to_memslot(vcpu, gfn), gfn);
 }
 EXPORT_SYMBOL_GPL(kvm_vcpu_gfn_to_pfn);
 
@@ -2115,18 +2123,20 @@ void kvm_release_pfn(kvm_pfn_t pfn, bool dirty, struct gfn_to_pfn_cache *cache)
 		kvm_release_pfn_clean(pfn);
 }
 
-static void kvm_cache_gfn_to_pfn(struct kvm_memory_slot *slot, gfn_t gfn,
+static void kvm_cache_gfn_to_pfn(struct kvm *kvm,
+				 struct kvm_memory_slot *slot, gfn_t gfn,
 				 struct gfn_to_pfn_cache *cache, u64 gen)
 {
 	kvm_release_pfn(cache->pfn, cache->dirty, cache);
 
-	cache->pfn = gfn_to_pfn_memslot(slot, gfn);
+	cache->pfn = gfn_to_pfn_memslot(kvm, slot, gfn);
 	cache->gfn = gfn;
 	cache->dirty = false;
 	cache->generation = gen;
 }
 
-static int __kvm_map_gfn(struct kvm_memslots *slots, gfn_t gfn,
+static int __kvm_map_gfn(struct kvm *kvm,
+			 struct kvm_memslots *slots, gfn_t gfn,
 			 struct kvm_host_map *map,
 			 struct gfn_to_pfn_cache *cache,
 			 bool atomic)
@@ -2145,13 +2155,13 @@ static int __kvm_map_gfn(struct kvm_memslots *slots, gfn_t gfn,
 			cache->generation != gen) {
 			if (atomic)
 				return -EAGAIN;
-			kvm_cache_gfn_to_pfn(slot, gfn, cache, gen);
+			kvm_cache_gfn_to_pfn(kvm, slot, gfn, cache, gen);
 		}
 		pfn = cache->pfn;
 	} else {
 		if (atomic)
 			return -EAGAIN;
-		pfn = gfn_to_pfn_memslot(slot, gfn);
+		pfn = gfn_to_pfn_memslot(kvm, slot, gfn);
 	}
 	if (is_error_noslot_pfn(pfn))
 		return -EINVAL;
@@ -2184,14 +2194,14 @@ static int __kvm_map_gfn(struct kvm_memslots *slots, gfn_t gfn,
 int kvm_map_gfn(struct kvm_vcpu *vcpu, gfn_t gfn, struct kvm_host_map *map,
 		struct gfn_to_pfn_cache *cache, bool atomic)
 {
-	return __kvm_map_gfn(kvm_memslots(vcpu->kvm), gfn, map,
+	return __kvm_map_gfn(vcpu->kvm, kvm_memslots(vcpu->kvm), gfn, map,
 			cache, atomic);
 }
 EXPORT_SYMBOL_GPL(kvm_map_gfn);
 
 int kvm_vcpu_map(struct kvm_vcpu *vcpu, gfn_t gfn, struct kvm_host_map *map)
 {
-	return __kvm_map_gfn(kvm_vcpu_memslots(vcpu), gfn, map,
+	return __kvm_map_gfn(vcpu->kvm, kvm_vcpu_memslots(vcpu), gfn, map,
 		NULL, false);
 }
 EXPORT_SYMBOL_GPL(kvm_vcpu_map);
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v7 17/17] mm: add mmu_notifier argument to follow_pfn
  2020-11-27 16:41 [PATCH v7 00/17] follow_pfn and other iomap races Daniel Vetter
                   ` (15 preceding siblings ...)
  2020-11-27 16:41 ` [PATCH v7 16/17] kvm: pass kvm argument to follow_pfn callsites Daniel Vetter
@ 2020-11-27 16:41 ` Daniel Vetter
  2020-11-27 19:10   ` kernel test robot
  2021-01-12 13:24 ` [PATCH v7 00/17] follow_pfn and other iomap races Daniel Vetter
  17 siblings, 1 reply; 32+ messages in thread
From: Daniel Vetter @ 2020-11-27 16:41 UTC (permalink / raw)
  To: DRI Development, LKML
  Cc: kvm, linux-mm, linux-arm-kernel, linux-samsung-soc, linux-media,
	Daniel Vetter, Daniel Vetter, Christoph Hellwig, Jason Gunthorpe,
	Kees Cook, Dan Williams, Andrew Morton, John Hubbard,
	Jérôme Glisse, Jan Kara

The only safe way for non core/arch code to use follow_pfn() is
together with an mmu_notifier subscription. follow_pfn() is already
marked as _GPL and the kerneldoc explains this restriction.

This patch here enforces all this by adding a mmu_notifier argument
and verifying that it is registered for the correct mm_struct.

Motivated by discussions with Christoph Hellwig and Jason Gunthorpe.

Since requiring an mmu_notifier makes it very clear that follow_pfn()
cannot be used on !CONFIG_MMU hardware, remove it from there. The sole
user kvm not existing on such hardware also supports that.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Christoph Hellwig <hch@infradead.org>
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
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
--
v7: Comments from Jason:
- ditch follow_pfn from nommu.c
- simplify mmu_notifer->mm check
---
 include/linux/mm.h  |  3 ++-
 mm/memory.c         | 38 ++++++++++++++++++++++++--------------
 mm/nommu.c          | 27 +++++----------------------
 virt/kvm/kvm_main.c |  4 ++--
 4 files changed, 33 insertions(+), 39 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index bb3e926afd91..2a564bfd818c 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1651,6 +1651,7 @@ void unmap_vmas(struct mmu_gather *tlb, struct vm_area_struct *start_vma,
 		unsigned long start, unsigned long end);
 
 struct mmu_notifier_range;
+struct mmu_notifier;
 
 void free_pgd_range(struct mmu_gather *tlb, unsigned long addr,
 		unsigned long end, unsigned long floor, unsigned long ceiling);
@@ -1660,7 +1661,7 @@ int follow_pte_pmd(struct mm_struct *mm, unsigned long address,
 		   struct mmu_notifier_range *range,
 		   pte_t **ptepp, pmd_t **pmdpp, spinlock_t **ptlp);
 int follow_pfn(struct vm_area_struct *vma, unsigned long address,
-	unsigned long *pfn);
+	unsigned long *pfn, struct mmu_notifier *subscription);
 int unsafe_follow_pfn(struct vm_area_struct *vma, unsigned long address,
 		      unsigned long *pfn);
 int follow_phys(struct vm_area_struct *vma, unsigned long address,
diff --git a/mm/memory.c b/mm/memory.c
index 0db0c5e233fd..a27b9b9c22c2 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4789,11 +4789,30 @@ int follow_pte_pmd(struct mm_struct *mm, unsigned long address,
 }
 EXPORT_SYMBOL(follow_pte_pmd);
 
+static int __follow_pfn(struct vm_area_struct *vma, unsigned long address,
+			unsigned long *pfn)
+{
+	int ret = -EINVAL;
+	spinlock_t *ptl;
+	pte_t *ptep;
+
+	if (!(vma->vm_flags & (VM_IO | VM_PFNMAP)))
+		return ret;
+
+	ret = follow_pte(vma->vm_mm, address, &ptep, &ptl);
+	if (ret)
+		return ret;
+	*pfn = pte_pfn(*ptep);
+	pte_unmap_unlock(ptep, ptl);
+	return 0;
+}
+
 /**
  * follow_pfn - look up PFN at a user virtual address
  * @vma: memory mapping
  * @address: user virtual address
  * @pfn: location to store found PFN
+ * @subscription: mmu_notifier subscription for the mm @vma is part of
  *
  * Only IO mappings and raw PFN mappings are allowed. Note that callers must
  * ensure coherency with pte updates by using a &mmu_notifier to follow updates.
@@ -4805,21 +4824,12 @@ EXPORT_SYMBOL(follow_pte_pmd);
  * Return: zero and the pfn at @pfn on success, -ve otherwise.
  */
 int follow_pfn(struct vm_area_struct *vma, unsigned long address,
-	unsigned long *pfn)
+	unsigned long *pfn, struct mmu_notifier *subscription)
 {
-	int ret = -EINVAL;
-	spinlock_t *ptl;
-	pte_t *ptep;
-
-	if (!(vma->vm_flags & (VM_IO | VM_PFNMAP)))
-		return ret;
+	if (WARN_ON(subscription->mm != vma->vm_mm))
+		return -EINVAL;
 
-	ret = follow_pte(vma->vm_mm, address, &ptep, &ptl);
-	if (ret)
-		return ret;
-	*pfn = pte_pfn(*ptep);
-	pte_unmap_unlock(ptep, ptl);
-	return 0;
+	return __follow_pfn(vma, address, pfn);
 }
 EXPORT_SYMBOL_GPL(follow_pfn);
 
@@ -4844,7 +4854,7 @@ int unsafe_follow_pfn(struct vm_area_struct *vma, unsigned long address,
 	WARN_ONCE(1, "unsafe follow_pfn usage\n");
 	add_taint(TAINT_USER, LOCKDEP_STILL_OK);
 
-	return follow_pfn(vma, address, pfn);
+	return __follow_pfn(vma, address, pfn);
 }
 EXPORT_SYMBOL(unsafe_follow_pfn);
 
diff --git a/mm/nommu.c b/mm/nommu.c
index 79fc98a6c94a..a1e178401146 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -111,27 +111,6 @@ unsigned int kobjsize(const void *objp)
 	return page_size(page);
 }
 
-/**
- * follow_pfn - look up PFN at a user virtual address
- * @vma: memory mapping
- * @address: user virtual address
- * @pfn: location to store found PFN
- *
- * Only IO mappings and raw PFN mappings are allowed.
- *
- * Returns zero and the pfn at @pfn on success, -ve otherwise.
- */
-int follow_pfn(struct vm_area_struct *vma, unsigned long address,
-	unsigned long *pfn)
-{
-	if (!(vma->vm_flags & (VM_IO | VM_PFNMAP)))
-		return -EINVAL;
-
-	*pfn = address >> PAGE_SHIFT;
-	return 0;
-}
-EXPORT_SYMBOL_GPL(follow_pfn);
-
 /**
  * unsafe_follow_pfn - look up PFN at a user virtual address
  * @vma: memory mapping
@@ -153,7 +132,11 @@ int unsafe_follow_pfn(struct vm_area_struct *vma, unsigned long address,
 	WARN_ONCE(1, "unsafe follow_pfn usage\n");
 	add_taint(TAINT_USER, LOCKDEP_STILL_OK);
 
-	return follow_pfn(vma, address, pfn);
+	if (!(vma->vm_flags & (VM_IO | VM_PFNMAP)))
+		return -EINVAL;
+
+	*pfn = address >> PAGE_SHIFT;
+	return 0;
 }
 EXPORT_SYMBOL(unsafe_follow_pfn);
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 417f3d470c3e..6f6786524eff 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1891,7 +1891,7 @@ static int hva_to_pfn_remapped(struct kvm *kvm, struct vm_area_struct *vma,
 	unsigned long pfn;
 	int r;
 
-	r = follow_pfn(vma, addr, &pfn);
+	r = follow_pfn(vma, addr, &pfn, &kvm->mmu_notifier);
 	if (r) {
 		/*
 		 * get_user_pages fails for VM_IO and VM_PFNMAP vmas and does
@@ -1906,7 +1906,7 @@ static int hva_to_pfn_remapped(struct kvm *kvm, struct vm_area_struct *vma,
 		if (r)
 			return r;
 
-		r = follow_pfn(vma, addr, &pfn);
+		r = follow_pfn(vma, addr, &pfn, &kvm->mmu_notifier);
 		if (r)
 			return r;
 
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH v7 17/17] mm: add mmu_notifier argument to follow_pfn
  2020-11-27 16:41 ` [PATCH v7 17/17] mm: add mmu_notifier argument to follow_pfn Daniel Vetter
@ 2020-11-27 19:10   ` kernel test robot
  2020-11-30 14:28     ` Daniel Vetter
  0 siblings, 1 reply; 32+ messages in thread
From: kernel test robot @ 2020-11-27 19:10 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development, LKML
  Cc: kbuild-all, clang-built-linux, kvm, linux-mm, linux-arm-kernel,
	linux-samsung-soc, linux-media, Daniel Vetter, Christoph Hellwig,
	Jason Gunthorpe

[-- Attachment #1: Type: text/plain, Size: 10718 bytes --]

Hi Daniel,

I love your patch! Yet something to improve:

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on char-misc/char-misc-testing v5.10-rc5]
[cannot apply to hnaz-linux-mm/master next-20201127]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Daniel-Vetter/follow_pfn-and-other-iomap-races/20201128-004421
base:   git://linuxtv.org/media_tree.git master
config: s390-randconfig-r032-20201127 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project f095ac11a9550530a4a54298debb8b04b36422be)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install s390 cross compiling tool for clang build
        # apt-get install binutils-s390x-linux-gnu
        # https://github.com/0day-ci/linux/commit/d76a3489433ce67d45da86aa12953385427f0ac9
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Daniel-Vetter/follow_pfn-and-other-iomap-races/20201128-004421
        git checkout d76a3489433ce67d45da86aa12953385427f0ac9
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from arch/s390/include/asm/kvm_para.h:25:
   In file included from arch/s390/include/asm/diag.h:12:
   In file included from include/linux/if_ether.h:19:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:80:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:21:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0x00ff0000UL) >>  8) |            \
                     ^
   In file included from arch/s390/kvm/../../../virt/kvm/kvm_main.c:18:
   In file included from include/linux/kvm_host.h:32:
   In file included from include/linux/kvm_para.h:5:
   In file included from include/uapi/linux/kvm_para.h:36:
   In file included from arch/s390/include/asm/kvm_para.h:25:
   In file included from arch/s390/include/asm/diag.h:12:
   In file included from include/linux/if_ether.h:19:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:80:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:22:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0xff000000UL) >> 24)))
                     ^
   In file included from arch/s390/kvm/../../../virt/kvm/kvm_main.c:18:
   In file included from include/linux/kvm_host.h:32:
   In file included from include/linux/kvm_para.h:5:
   In file included from include/uapi/linux/kvm_para.h:36:
   In file included from arch/s390/include/asm/kvm_para.h:25:
   In file included from arch/s390/include/asm/diag.h:12:
   In file included from include/linux/if_ether.h:19:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:80:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:120:12: note: expanded from macro '__swab32'
           __fswab32(x))
                     ^
   In file included from arch/s390/kvm/../../../virt/kvm/kvm_main.c:18:
   In file included from include/linux/kvm_host.h:32:
   In file included from include/linux/kvm_para.h:5:
   In file included from include/uapi/linux/kvm_para.h:36:
   In file included from arch/s390/include/asm/kvm_para.h:25:
   In file included from arch/s390/include/asm/diag.h:12:
   In file included from include/linux/if_ether.h:19:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:80:
   include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
>> arch/s390/kvm/../../../virt/kvm/kvm_main.c:1894:40: error: no member named 'mmu_notifier' in 'struct kvm'
           r = follow_pfn(vma, addr, &pfn, &kvm->mmu_notifier);
                                            ~~~  ^
   arch/s390/kvm/../../../virt/kvm/kvm_main.c:1909:41: error: no member named 'mmu_notifier' in 'struct kvm'
                   r = follow_pfn(vma, addr, &pfn, &kvm->mmu_notifier);
                                                    ~~~  ^
   20 warnings and 2 errors generated.

vim +1894 arch/s390/kvm/../../../virt/kvm/kvm_main.c

  1885	
  1886	static int hva_to_pfn_remapped(struct kvm *kvm, struct vm_area_struct *vma,
  1887				       unsigned long addr, bool *async,
  1888				       bool write_fault, bool *writable,
  1889				       kvm_pfn_t *p_pfn)
  1890	{
  1891		unsigned long pfn;
  1892		int r;
  1893	
> 1894		r = follow_pfn(vma, addr, &pfn, &kvm->mmu_notifier);
  1895		if (r) {
  1896			/*
  1897			 * get_user_pages fails for VM_IO and VM_PFNMAP vmas and does
  1898			 * not call the fault handler, so do it here.
  1899			 */
  1900			bool unlocked = false;
  1901			r = fixup_user_fault(current->mm, addr,
  1902					     (write_fault ? FAULT_FLAG_WRITE : 0),
  1903					     &unlocked);
  1904			if (unlocked)
  1905				return -EAGAIN;
  1906			if (r)
  1907				return r;
  1908	
  1909			r = follow_pfn(vma, addr, &pfn, &kvm->mmu_notifier);
  1910			if (r)
  1911				return r;
  1912	
  1913		}
  1914	
  1915		if (writable)
  1916			*writable = true;
  1917	
  1918		/*
  1919		 * Get a reference here because callers of *hva_to_pfn* and
  1920		 * *gfn_to_pfn* ultimately call kvm_release_pfn_clean on the
  1921		 * returned pfn.  This is only needed if the VMA has VM_MIXEDMAP
  1922		 * set, but the kvm_get_pfn/kvm_release_pfn_clean pair will
  1923		 * simply do nothing for reserved pfns.
  1924		 *
  1925		 * Whoever called remap_pfn_range is also going to call e.g.
  1926		 * unmap_mapping_range before the underlying pages are freed,
  1927		 * causing a call to our MMU notifier.
  1928		 */ 
  1929		kvm_get_pfn(pfn);
  1930	
  1931		*p_pfn = pfn;
  1932		return 0;
  1933	}
  1934	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 20372 bytes --]

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v7 17/17] mm: add mmu_notifier argument to follow_pfn
  2020-11-27 19:10   ` kernel test robot
@ 2020-11-30 14:28     ` Daniel Vetter
  2020-11-30 18:03       ` Nick Desaulniers
  0 siblings, 1 reply; 32+ messages in thread
From: Daniel Vetter @ 2020-11-30 14:28 UTC (permalink / raw)
  To: kernel test robot
  Cc: Daniel Vetter, DRI Development, LKML, kbuild-all,
	clang-built-linux, kvm, linux-mm, linux-arm-kernel,
	linux-samsung-soc, linux-media, Christoph Hellwig,
	Jason Gunthorpe

So I guess kvm platforms that don't set KVM_ARCH_WANT_MMU_NOTIFIER exist,
and at least on powerpc they're consistent with KVM_CAP_SYNC_MMU
signalling that the guest pagetables stays in sync automatically with any
updates. So for that case I guess we could use unsafe_follow_pfn.

But on s390 this seems different: No mmu notifier, but KVM_CAP_SYNC_MMU is
set. So I guess there's some hardware magic on s390 that I don't know
about.

Not sure what to do with this now here ...
-Daniel


On Sat, Nov 28, 2020 at 03:10:40AM +0800, kernel test robot wrote:
> Hi Daniel,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on linuxtv-media/master]
> [also build test ERROR on char-misc/char-misc-testing v5.10-rc5]
> [cannot apply to hnaz-linux-mm/master next-20201127]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Daniel-Vetter/follow_pfn-and-other-iomap-races/20201128-004421
> base:   git://linuxtv.org/media_tree.git master
> config: s390-randconfig-r032-20201127 (attached as .config)
> compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project f095ac11a9550530a4a54298debb8b04b36422be)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # install s390 cross compiling tool for clang build
>         # apt-get install binutils-s390x-linux-gnu
>         # https://github.com/0day-ci/linux/commit/d76a3489433ce67d45da86aa12953385427f0ac9
>         git remote add linux-review https://github.com/0day-ci/linux
>         git fetch --no-tags linux-review Daniel-Vetter/follow_pfn-and-other-iomap-races/20201128-004421
>         git checkout d76a3489433ce67d45da86aa12953385427f0ac9
>         # save the attached .config to linux build tree
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>    In file included from arch/s390/include/asm/kvm_para.h:25:
>    In file included from arch/s390/include/asm/diag.h:12:
>    In file included from include/linux/if_ether.h:19:
>    In file included from include/linux/skbuff.h:31:
>    In file included from include/linux/dma-mapping.h:10:
>    In file included from include/linux/scatterlist.h:9:
>    In file included from arch/s390/include/asm/io.h:80:
>    include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
>            val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
>                                                            ~~~~~~~~~~ ^
>    include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
>    #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
>                                                              ^
>    include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
>            ___constant_swab32(x) :                 \
>                               ^
>    include/uapi/linux/swab.h:21:12: note: expanded from macro '___constant_swab32'
>            (((__u32)(x) & (__u32)0x00ff0000UL) >>  8) |            \
>                      ^
>    In file included from arch/s390/kvm/../../../virt/kvm/kvm_main.c:18:
>    In file included from include/linux/kvm_host.h:32:
>    In file included from include/linux/kvm_para.h:5:
>    In file included from include/uapi/linux/kvm_para.h:36:
>    In file included from arch/s390/include/asm/kvm_para.h:25:
>    In file included from arch/s390/include/asm/diag.h:12:
>    In file included from include/linux/if_ether.h:19:
>    In file included from include/linux/skbuff.h:31:
>    In file included from include/linux/dma-mapping.h:10:
>    In file included from include/linux/scatterlist.h:9:
>    In file included from arch/s390/include/asm/io.h:80:
>    include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
>            val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
>                                                            ~~~~~~~~~~ ^
>    include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
>    #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
>                                                              ^
>    include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
>            ___constant_swab32(x) :                 \
>                               ^
>    include/uapi/linux/swab.h:22:12: note: expanded from macro '___constant_swab32'
>            (((__u32)(x) & (__u32)0xff000000UL) >> 24)))
>                      ^
>    In file included from arch/s390/kvm/../../../virt/kvm/kvm_main.c:18:
>    In file included from include/linux/kvm_host.h:32:
>    In file included from include/linux/kvm_para.h:5:
>    In file included from include/uapi/linux/kvm_para.h:36:
>    In file included from arch/s390/include/asm/kvm_para.h:25:
>    In file included from arch/s390/include/asm/diag.h:12:
>    In file included from include/linux/if_ether.h:19:
>    In file included from include/linux/skbuff.h:31:
>    In file included from include/linux/dma-mapping.h:10:
>    In file included from include/linux/scatterlist.h:9:
>    In file included from arch/s390/include/asm/io.h:80:
>    include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
>            val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
>                                                            ~~~~~~~~~~ ^
>    include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
>    #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
>                                                              ^
>    include/uapi/linux/swab.h:120:12: note: expanded from macro '__swab32'
>            __fswab32(x))
>                      ^
>    In file included from arch/s390/kvm/../../../virt/kvm/kvm_main.c:18:
>    In file included from include/linux/kvm_host.h:32:
>    In file included from include/linux/kvm_para.h:5:
>    In file included from include/uapi/linux/kvm_para.h:36:
>    In file included from arch/s390/include/asm/kvm_para.h:25:
>    In file included from arch/s390/include/asm/diag.h:12:
>    In file included from include/linux/if_ether.h:19:
>    In file included from include/linux/skbuff.h:31:
>    In file included from include/linux/dma-mapping.h:10:
>    In file included from include/linux/scatterlist.h:9:
>    In file included from arch/s390/include/asm/io.h:80:
>    include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
>            __raw_writeb(value, PCI_IOBASE + addr);
>                                ~~~~~~~~~~ ^
>    include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
>            __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
>                                                          ~~~~~~~~~~ ^
>    include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
>            __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
>                                                          ~~~~~~~~~~ ^
>    include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
>            readsb(PCI_IOBASE + addr, buffer, count);
>                   ~~~~~~~~~~ ^
>    include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
>            readsw(PCI_IOBASE + addr, buffer, count);
>                   ~~~~~~~~~~ ^
>    include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
>            readsl(PCI_IOBASE + addr, buffer, count);
>                   ~~~~~~~~~~ ^
>    include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
>            writesb(PCI_IOBASE + addr, buffer, count);
>                    ~~~~~~~~~~ ^
>    include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
>            writesw(PCI_IOBASE + addr, buffer, count);
>                    ~~~~~~~~~~ ^
>    include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
>            writesl(PCI_IOBASE + addr, buffer, count);
>                    ~~~~~~~~~~ ^
> >> arch/s390/kvm/../../../virt/kvm/kvm_main.c:1894:40: error: no member named 'mmu_notifier' in 'struct kvm'
>            r = follow_pfn(vma, addr, &pfn, &kvm->mmu_notifier);
>                                             ~~~  ^
>    arch/s390/kvm/../../../virt/kvm/kvm_main.c:1909:41: error: no member named 'mmu_notifier' in 'struct kvm'
>                    r = follow_pfn(vma, addr, &pfn, &kvm->mmu_notifier);
>                                                     ~~~  ^
>    20 warnings and 2 errors generated.
> 
> vim +1894 arch/s390/kvm/../../../virt/kvm/kvm_main.c
> 
>   1885	
>   1886	static int hva_to_pfn_remapped(struct kvm *kvm, struct vm_area_struct *vma,
>   1887				       unsigned long addr, bool *async,
>   1888				       bool write_fault, bool *writable,
>   1889				       kvm_pfn_t *p_pfn)
>   1890	{
>   1891		unsigned long pfn;
>   1892		int r;
>   1893	
> > 1894		r = follow_pfn(vma, addr, &pfn, &kvm->mmu_notifier);
>   1895		if (r) {
>   1896			/*
>   1897			 * get_user_pages fails for VM_IO and VM_PFNMAP vmas and does
>   1898			 * not call the fault handler, so do it here.
>   1899			 */
>   1900			bool unlocked = false;
>   1901			r = fixup_user_fault(current->mm, addr,
>   1902					     (write_fault ? FAULT_FLAG_WRITE : 0),
>   1903					     &unlocked);
>   1904			if (unlocked)
>   1905				return -EAGAIN;
>   1906			if (r)
>   1907				return r;
>   1908	
>   1909			r = follow_pfn(vma, addr, &pfn, &kvm->mmu_notifier);
>   1910			if (r)
>   1911				return r;
>   1912	
>   1913		}
>   1914	
>   1915		if (writable)
>   1916			*writable = true;
>   1917	
>   1918		/*
>   1919		 * Get a reference here because callers of *hva_to_pfn* and
>   1920		 * *gfn_to_pfn* ultimately call kvm_release_pfn_clean on the
>   1921		 * returned pfn.  This is only needed if the VMA has VM_MIXEDMAP
>   1922		 * set, but the kvm_get_pfn/kvm_release_pfn_clean pair will
>   1923		 * simply do nothing for reserved pfns.
>   1924		 *
>   1925		 * Whoever called remap_pfn_range is also going to call e.g.
>   1926		 * unmap_mapping_range before the underlying pages are freed,
>   1927		 * causing a call to our MMU notifier.
>   1928		 */ 
>   1929		kvm_get_pfn(pfn);
>   1930	
>   1931		*p_pfn = pfn;
>   1932		return 0;
>   1933	}
>   1934	
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org



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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v7 17/17] mm: add mmu_notifier argument to follow_pfn
  2020-11-30 14:28     ` Daniel Vetter
@ 2020-11-30 18:03       ` Nick Desaulniers
  0 siblings, 0 replies; 32+ messages in thread
From: Nick Desaulniers @ 2020-11-30 18:03 UTC (permalink / raw)
  To: Vasily Gorbik, linux-s390, Heiko Carstens
  Cc: Daniel Vetter, DRI Development, LKML, kbuild-all,
	clang-built-linux, kvm, Linux Memory Management List, Linux ARM,
	linux-samsung-soc, linux-media, Christoph Hellwig,
	Jason Gunthorpe, kernel test robot

On Mon, Nov 30, 2020 at 6:28 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> So I guess kvm platforms that don't set KVM_ARCH_WANT_MMU_NOTIFIER exist,
> and at least on powerpc they're consistent with KVM_CAP_SYNC_MMU
> signalling that the guest pagetables stays in sync automatically with any
> updates. So for that case I guess we could use unsafe_follow_pfn.
>
> But on s390 this seems different: No mmu notifier, but KVM_CAP_SYNC_MMU is
> set. So I guess there's some hardware magic on s390 that I don't know
> about.

+ Vasily + Heiko +s390

>
> Not sure what to do with this now here ...
> -Daniel
>
>
> On Sat, Nov 28, 2020 at 03:10:40AM +0800, kernel test robot wrote:
> > Hi Daniel,
> >
> > I love your patch! Yet something to improve:
> >
> > [auto build test ERROR on linuxtv-media/master]
> > [also build test ERROR on char-misc/char-misc-testing v5.10-rc5]
> > [cannot apply to hnaz-linux-mm/master next-20201127]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch]
> >
> > url:    https://github.com/0day-ci/linux/commits/Daniel-Vetter/follow_pfn-and-other-iomap-races/20201128-004421
> > base:   git://linuxtv.org/media_tree.git master
> > config: s390-randconfig-r032-20201127 (attached as .config)
> > compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project f095ac11a9550530a4a54298debb8b04b36422be)
> > reproduce (this is a W=1 build):
> >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         # install s390 cross compiling tool for clang build
> >         # apt-get install binutils-s390x-linux-gnu
> >         # https://github.com/0day-ci/linux/commit/d76a3489433ce67d45da86aa12953385427f0ac9
> >         git remote add linux-review https://github.com/0day-ci/linux
> >         git fetch --no-tags linux-review Daniel-Vetter/follow_pfn-and-other-iomap-races/20201128-004421
> >         git checkout d76a3489433ce67d45da86aa12953385427f0ac9
> >         # save the attached .config to linux build tree
> >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390
> >
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <lkp@intel.com>
> >
> > All errors (new ones prefixed by >>):
> >
> >    In file included from arch/s390/include/asm/kvm_para.h:25:
> >    In file included from arch/s390/include/asm/diag.h:12:
> >    In file included from include/linux/if_ether.h:19:
> >    In file included from include/linux/skbuff.h:31:
> >    In file included from include/linux/dma-mapping.h:10:
> >    In file included from include/linux/scatterlist.h:9:
> >    In file included from arch/s390/include/asm/io.h:80:
> >    include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
> >            val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
> >                                                            ~~~~~~~~~~ ^
> >    include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
> >    #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
> >                                                              ^
> >    include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
> >            ___constant_swab32(x) :                 \
> >                               ^
> >    include/uapi/linux/swab.h:21:12: note: expanded from macro '___constant_swab32'
> >            (((__u32)(x) & (__u32)0x00ff0000UL) >>  8) |            \
> >                      ^
> >    In file included from arch/s390/kvm/../../../virt/kvm/kvm_main.c:18:
> >    In file included from include/linux/kvm_host.h:32:
> >    In file included from include/linux/kvm_para.h:5:
> >    In file included from include/uapi/linux/kvm_para.h:36:
> >    In file included from arch/s390/include/asm/kvm_para.h:25:
> >    In file included from arch/s390/include/asm/diag.h:12:
> >    In file included from include/linux/if_ether.h:19:
> >    In file included from include/linux/skbuff.h:31:
> >    In file included from include/linux/dma-mapping.h:10:
> >    In file included from include/linux/scatterlist.h:9:
> >    In file included from arch/s390/include/asm/io.h:80:
> >    include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
> >            val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
> >                                                            ~~~~~~~~~~ ^
> >    include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
> >    #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
> >                                                              ^
> >    include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
> >            ___constant_swab32(x) :                 \
> >                               ^
> >    include/uapi/linux/swab.h:22:12: note: expanded from macro '___constant_swab32'
> >            (((__u32)(x) & (__u32)0xff000000UL) >> 24)))
> >                      ^
> >    In file included from arch/s390/kvm/../../../virt/kvm/kvm_main.c:18:
> >    In file included from include/linux/kvm_host.h:32:
> >    In file included from include/linux/kvm_para.h:5:
> >    In file included from include/uapi/linux/kvm_para.h:36:
> >    In file included from arch/s390/include/asm/kvm_para.h:25:
> >    In file included from arch/s390/include/asm/diag.h:12:
> >    In file included from include/linux/if_ether.h:19:
> >    In file included from include/linux/skbuff.h:31:
> >    In file included from include/linux/dma-mapping.h:10:
> >    In file included from include/linux/scatterlist.h:9:
> >    In file included from arch/s390/include/asm/io.h:80:
> >    include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
> >            val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
> >                                                            ~~~~~~~~~~ ^
> >    include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
> >    #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
> >                                                              ^
> >    include/uapi/linux/swab.h:120:12: note: expanded from macro '__swab32'
> >            __fswab32(x))
> >                      ^
> >    In file included from arch/s390/kvm/../../../virt/kvm/kvm_main.c:18:
> >    In file included from include/linux/kvm_host.h:32:
> >    In file included from include/linux/kvm_para.h:5:
> >    In file included from include/uapi/linux/kvm_para.h:36:
> >    In file included from arch/s390/include/asm/kvm_para.h:25:
> >    In file included from arch/s390/include/asm/diag.h:12:
> >    In file included from include/linux/if_ether.h:19:
> >    In file included from include/linux/skbuff.h:31:
> >    In file included from include/linux/dma-mapping.h:10:
> >    In file included from include/linux/scatterlist.h:9:
> >    In file included from arch/s390/include/asm/io.h:80:
> >    include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
> >            __raw_writeb(value, PCI_IOBASE + addr);
> >                                ~~~~~~~~~~ ^
> >    include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
> >            __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
> >                                                          ~~~~~~~~~~ ^
> >    include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
> >            __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
> >                                                          ~~~~~~~~~~ ^
> >    include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
> >            readsb(PCI_IOBASE + addr, buffer, count);
> >                   ~~~~~~~~~~ ^
> >    include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
> >            readsw(PCI_IOBASE + addr, buffer, count);
> >                   ~~~~~~~~~~ ^
> >    include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
> >            readsl(PCI_IOBASE + addr, buffer, count);
> >                   ~~~~~~~~~~ ^
> >    include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
> >            writesb(PCI_IOBASE + addr, buffer, count);
> >                    ~~~~~~~~~~ ^
> >    include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
> >            writesw(PCI_IOBASE + addr, buffer, count);
> >                    ~~~~~~~~~~ ^
> >    include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
> >            writesl(PCI_IOBASE + addr, buffer, count);
> >                    ~~~~~~~~~~ ^
> > >> arch/s390/kvm/../../../virt/kvm/kvm_main.c:1894:40: error: no member named 'mmu_notifier' in 'struct kvm'
> >            r = follow_pfn(vma, addr, &pfn, &kvm->mmu_notifier);
> >                                             ~~~  ^
> >    arch/s390/kvm/../../../virt/kvm/kvm_main.c:1909:41: error: no member named 'mmu_notifier' in 'struct kvm'
> >                    r = follow_pfn(vma, addr, &pfn, &kvm->mmu_notifier);
> >                                                     ~~~  ^
> >    20 warnings and 2 errors generated.
> >
> > vim +1894 arch/s390/kvm/../../../virt/kvm/kvm_main.c
> >
> >   1885
> >   1886        static int hva_to_pfn_remapped(struct kvm *kvm, struct vm_area_struct *vma,
> >   1887                                       unsigned long addr, bool *async,
> >   1888                                       bool write_fault, bool *writable,
> >   1889                                       kvm_pfn_t *p_pfn)
> >   1890        {
> >   1891                unsigned long pfn;
> >   1892                int r;
> >   1893
> > > 1894                r = follow_pfn(vma, addr, &pfn, &kvm->mmu_notifier);
> >   1895                if (r) {
> >   1896                        /*
> >   1897                         * get_user_pages fails for VM_IO and VM_PFNMAP vmas and does
> >   1898                         * not call the fault handler, so do it here.
> >   1899                         */
> >   1900                        bool unlocked = false;
> >   1901                        r = fixup_user_fault(current->mm, addr,
> >   1902                                             (write_fault ? FAULT_FLAG_WRITE : 0),
> >   1903                                             &unlocked);
> >   1904                        if (unlocked)
> >   1905                                return -EAGAIN;
> >   1906                        if (r)
> >   1907                                return r;
> >   1908
> >   1909                        r = follow_pfn(vma, addr, &pfn, &kvm->mmu_notifier);
> >   1910                        if (r)
> >   1911                                return r;
> >   1912
> >   1913                }
> >   1914
> >   1915                if (writable)
> >   1916                        *writable = true;
> >   1917
> >   1918                /*
> >   1919                 * Get a reference here because callers of *hva_to_pfn* and
> >   1920                 * *gfn_to_pfn* ultimately call kvm_release_pfn_clean on the
> >   1921                 * returned pfn.  This is only needed if the VMA has VM_MIXEDMAP
> >   1922                 * set, but the kvm_get_pfn/kvm_release_pfn_clean pair will
> >   1923                 * simply do nothing for reserved pfns.
> >   1924                 *
> >   1925                 * Whoever called remap_pfn_range is also going to call e.g.
> >   1926                 * unmap_mapping_range before the underlying pages are freed,
> >   1927                 * causing a call to our MMU notifier.
> >   1928                 */
> >   1929                kvm_get_pfn(pfn);
> >   1930
> >   1931                *p_pfn = pfn;
> >   1932                return 0;
> >   1933        }
> >   1934
> >
> > ---
> > 0-DAY CI Kernel Test Service, Intel Corporation
> > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20201130142820.GN401619%40phenom.ffwll.local.



-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v7 00/17] follow_pfn and other iomap races
  2020-11-27 16:41 [PATCH v7 00/17] follow_pfn and other iomap races Daniel Vetter
                   ` (16 preceding siblings ...)
  2020-11-27 16:41 ` [PATCH v7 17/17] mm: add mmu_notifier argument to follow_pfn Daniel Vetter
@ 2021-01-12 13:24 ` Daniel Vetter
  2021-01-12 13:28   ` Daniel Vetter
  2021-01-12 20:57   ` Stephen Rothwell
  17 siblings, 2 replies; 32+ messages in thread
From: Daniel Vetter @ 2021-01-12 13:24 UTC (permalink / raw)
  To: DRI Development, LKML, Stephen Rothwell
  Cc: linux-samsung-soc, kvm, Daniel Vetter, linux-mm,
	linux-arm-kernel, linux-media

On Fri, Nov 27, 2020 at 05:41:14PM +0100, Daniel Vetter wrote:
> Hi all
> 
> Another update of my patch series to clamp down a bunch of races and gaps
> around follow_pfn and other access to iomem mmaps. Previous version:
> 
> v1: https://lore.kernel.org/dri-devel/20201007164426.1812530-1-daniel.vetter@ffwll.ch/
> v2: https://lore.kernel.org/dri-devel/20201009075934.3509076-1-daniel.vetter@ffwll.ch
> v3: https://lore.kernel.org/dri-devel/20201021085655.1192025-1-daniel.vetter@ffwll.ch/
> v4: https://lore.kernel.org/dri-devel/20201026105818.2585306-1-daniel.vetter@ffwll.ch/
> v5: https://lore.kernel.org/dri-devel/20201030100815.2269-1-daniel.vetter@ffwll.ch/
> v6: https://lore.kernel.org/dri-devel/20201119144146.1045202-1-daniel.vetter@ffwll.ch/
> 
> And the discussion that sparked this journey:
> 
> https://lore.kernel.org/dri-devel/20201007164426.1812530-1-daniel.vetter@ffwll.ch/
> 
> I think the first 12 patches are ready for landing. The parts starting
> with "mm: Add unsafe_follow_pfn" probably need more baking time.
> 
> Andrew, can you please pick these up, or do you prefer I do a topic branch
> and send them to Linus directly in the next merge window?
> 
> Changes in v7:
> - more acks/reviews
> - reordered with the ready pieces at the front
> - simplified the new follow_pfn function as Jason suggested
> 
> Changes in v6:
> - Tested v4l userptr as Tomasz suggested. No boom observed
> - Added RFC for locking down follow_pfn, per discussion with Christoph and
>   Jason.
> - Explain why pup_fast is safe in relevant patches, there was a bit a
>   confusion when discussing v5.
> - Fix up the resource patch, with CONFIG_IO_STRICT_DEVMEM it crashed on
>   boot due to an unintended change (reported by John)
> 
> Changes in v5:
> - Tomasz found some issues in the media patches
> - Polish suggested by Christoph for the unsafe_follow_pfn patch
> 
> Changes in v4:
> - Drop the s390 patch, that was very stand-alone and now queued up to land
>   through s390 trees.
> - Comment polish per Dan's review.
> 
> Changes in v3:
> - Bunch of polish all over, no functional changes aside from one barrier
>   in the resource code, for consistency.
> - A few more r-b tags.
> 
> Changes in v2:
> - tons of small polish&fixes all over, thanks to all the reviewers who
>   spotted issues
> - I managed to test at least the generic_access_phys and pci mmap revoke
>   stuff with a few gdb sessions using our i915 debug tools (hence now also
>   the drm/i915 patch to properly request all the pci bar regions)
> - reworked approach for the pci mmap revoke: Infrastructure moved into
>   kernel/resource.c, address_space mapping is now set up at open time for
>   everyone (which required some sysfs changes). Does indeed look a lot
>   cleaner and a lot less invasive than I feared at first.
> 
> Coments and review on the remaining bits very much welcome, especially
> from the kvm and vfio side.
> 
> Cheers, Daniel
> 
> Daniel Vetter (17):
>   drm/exynos: Stop using frame_vector helpers
>   drm/exynos: Use FOLL_LONGTERM for g2d cmdlists
>   misc/habana: Stop using frame_vector helpers
>   misc/habana: Use FOLL_LONGTERM for userptr
>   mm/frame-vector: Use FOLL_LONGTERM
>   media: videobuf2: Move frame_vector into media subsystem
>   mm: Close race in generic_access_phys
>   PCI: Obey iomem restrictions for procfs mmap
>   /dev/mem: Only set filp->f_mapping
>   resource: Move devmem revoke code to resource framework
>   sysfs: Support zapping of binary attr mmaps
>   PCI: Revoke mappings like devmem

As Jason suggested, I've pulled the first 1 patches into a topic branch.

Stephen, can you please add the below to linux-next for the 5.12 merge
window?

git://anongit.freedesktop.org/drm/drm topic/iomem-mmap-vs-gup

Once this part has landed I'll see what to do with the below part.

Thanks, Daniel

>   mm: Add unsafe_follow_pfn
>   media/videobuf1|2: Mark follow_pfn usage as unsafe
>   vfio/type1: Mark follow_pfn as unsafe
>   kvm: pass kvm argument to follow_pfn callsites
>   mm: add mmu_notifier argument to follow_pfn
> 
>  arch/powerpc/kvm/book3s_64_mmu_hv.c           |   2 +-
>  arch/powerpc/kvm/book3s_64_mmu_radix.c        |   2 +-
>  arch/powerpc/kvm/e500_mmu_host.c              |   2 +-
>  arch/x86/kvm/mmu/mmu.c                        |   8 +-
>  drivers/char/mem.c                            |  86 +-------------
>  drivers/gpu/drm/exynos/Kconfig                |   1 -
>  drivers/gpu/drm/exynos/exynos_drm_g2d.c       |  48 ++++----
>  drivers/media/common/videobuf2/Kconfig        |   1 -
>  drivers/media/common/videobuf2/Makefile       |   1 +
>  .../media/common/videobuf2}/frame_vector.c    |  57 ++++-----
>  .../media/common/videobuf2/videobuf2-memops.c |   3 +-
>  drivers/media/platform/omap/Kconfig           |   1 -
>  drivers/media/v4l2-core/videobuf-dma-contig.c |   2 +-
>  drivers/misc/habanalabs/Kconfig               |   1 -
>  drivers/misc/habanalabs/common/habanalabs.h   |   6 +-
>  drivers/misc/habanalabs/common/memory.c       |  52 +++-----
>  drivers/pci/pci-sysfs.c                       |   4 +
>  drivers/pci/proc.c                            |   6 +
>  drivers/vfio/vfio_iommu_type1.c               |   4 +-
>  fs/sysfs/file.c                               |  11 ++
>  include/linux/ioport.h                        |   6 +-
>  include/linux/kvm_host.h                      |   9 +-
>  include/linux/mm.h                            |  50 +-------
>  include/linux/sysfs.h                         |   2 +
>  include/media/frame_vector.h                  |  47 ++++++++
>  include/media/videobuf2-core.h                |   1 +
>  kernel/resource.c                             |  98 ++++++++++++++-
>  mm/Kconfig                                    |   3 -
>  mm/Makefile                                   |   1 -
>  mm/memory.c                                   | 112 +++++++++++++++---
>  mm/nommu.c                                    |  16 ++-
>  security/Kconfig                              |  13 ++
>  virt/kvm/kvm_main.c                           |  56 +++++----
>  33 files changed, 413 insertions(+), 299 deletions(-)
>  rename {mm => drivers/media/common/videobuf2}/frame_vector.c (84%)
>  create mode 100644 include/media/frame_vector.h
> 
> -- 
> 2.29.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v7 00/17] follow_pfn and other iomap races
  2021-01-12 13:24 ` [PATCH v7 00/17] follow_pfn and other iomap races Daniel Vetter
@ 2021-01-12 13:28   ` Daniel Vetter
  2021-01-12 20:57   ` Stephen Rothwell
  1 sibling, 0 replies; 32+ messages in thread
From: Daniel Vetter @ 2021-01-12 13:28 UTC (permalink / raw)
  To: DRI Development
  Cc: LKML, Stephen Rothwell, linux-samsung-soc, KVM list, Linux MM,
	Linux ARM, open list:DMA BUFFER SHARING FRAMEWORK

On Tue, Jan 12, 2021 at 2:24 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Fri, Nov 27, 2020 at 05:41:14PM +0100, Daniel Vetter wrote:
> > Hi all
> >
> > Another update of my patch series to clamp down a bunch of races and gaps
> > around follow_pfn and other access to iomem mmaps. Previous version:
> >
> > v1: https://lore.kernel.org/dri-devel/20201007164426.1812530-1-daniel.vetter@ffwll.ch/
> > v2: https://lore.kernel.org/dri-devel/20201009075934.3509076-1-daniel.vetter@ffwll.ch
> > v3: https://lore.kernel.org/dri-devel/20201021085655.1192025-1-daniel.vetter@ffwll.ch/
> > v4: https://lore.kernel.org/dri-devel/20201026105818.2585306-1-daniel.vetter@ffwll.ch/
> > v5: https://lore.kernel.org/dri-devel/20201030100815.2269-1-daniel.vetter@ffwll.ch/
> > v6: https://lore.kernel.org/dri-devel/20201119144146.1045202-1-daniel.vetter@ffwll.ch/
> >
> > And the discussion that sparked this journey:
> >
> > https://lore.kernel.org/dri-devel/20201007164426.1812530-1-daniel.vetter@ffwll.ch/
> >
> > I think the first 12 patches are ready for landing. The parts starting
> > with "mm: Add unsafe_follow_pfn" probably need more baking time.
> >
> > Andrew, can you please pick these up, or do you prefer I do a topic branch
> > and send them to Linus directly in the next merge window?
> >
> > Changes in v7:
> > - more acks/reviews
> > - reordered with the ready pieces at the front
> > - simplified the new follow_pfn function as Jason suggested
> >
> > Changes in v6:
> > - Tested v4l userptr as Tomasz suggested. No boom observed
> > - Added RFC for locking down follow_pfn, per discussion with Christoph and
> >   Jason.
> > - Explain why pup_fast is safe in relevant patches, there was a bit a
> >   confusion when discussing v5.
> > - Fix up the resource patch, with CONFIG_IO_STRICT_DEVMEM it crashed on
> >   boot due to an unintended change (reported by John)
> >
> > Changes in v5:
> > - Tomasz found some issues in the media patches
> > - Polish suggested by Christoph for the unsafe_follow_pfn patch
> >
> > Changes in v4:
> > - Drop the s390 patch, that was very stand-alone and now queued up to land
> >   through s390 trees.
> > - Comment polish per Dan's review.
> >
> > Changes in v3:
> > - Bunch of polish all over, no functional changes aside from one barrier
> >   in the resource code, for consistency.
> > - A few more r-b tags.
> >
> > Changes in v2:
> > - tons of small polish&fixes all over, thanks to all the reviewers who
> >   spotted issues
> > - I managed to test at least the generic_access_phys and pci mmap revoke
> >   stuff with a few gdb sessions using our i915 debug tools (hence now also
> >   the drm/i915 patch to properly request all the pci bar regions)
> > - reworked approach for the pci mmap revoke: Infrastructure moved into
> >   kernel/resource.c, address_space mapping is now set up at open time for
> >   everyone (which required some sysfs changes). Does indeed look a lot
> >   cleaner and a lot less invasive than I feared at first.
> >
> > Coments and review on the remaining bits very much welcome, especially
> > from the kvm and vfio side.
> >
> > Cheers, Daniel
> >
> > Daniel Vetter (17):
> >   drm/exynos: Stop using frame_vector helpers
> >   drm/exynos: Use FOLL_LONGTERM for g2d cmdlists
> >   misc/habana: Stop using frame_vector helpers
> >   misc/habana: Use FOLL_LONGTERM for userptr
> >   mm/frame-vector: Use FOLL_LONGTERM
> >   media: videobuf2: Move frame_vector into media subsystem
> >   mm: Close race in generic_access_phys
> >   PCI: Obey iomem restrictions for procfs mmap
> >   /dev/mem: Only set filp->f_mapping
> >   resource: Move devmem revoke code to resource framework
> >   sysfs: Support zapping of binary attr mmaps
> >   PCI: Revoke mappings like devmem
>
> As Jason suggested, I've pulled the first 1 patches into a topic branch.

Uh this was meant to read "first _12_ patches" ofc.
-Daniel

>
> Stephen, can you please add the below to linux-next for the 5.12 merge
> window?
>
> git://anongit.freedesktop.org/drm/drm topic/iomem-mmap-vs-gup
>
> Once this part has landed I'll see what to do with the below part.
>
> Thanks, Daniel
>
> >   mm: Add unsafe_follow_pfn
> >   media/videobuf1|2: Mark follow_pfn usage as unsafe
> >   vfio/type1: Mark follow_pfn as unsafe
> >   kvm: pass kvm argument to follow_pfn callsites
> >   mm: add mmu_notifier argument to follow_pfn
> >
> >  arch/powerpc/kvm/book3s_64_mmu_hv.c           |   2 +-
> >  arch/powerpc/kvm/book3s_64_mmu_radix.c        |   2 +-
> >  arch/powerpc/kvm/e500_mmu_host.c              |   2 +-
> >  arch/x86/kvm/mmu/mmu.c                        |   8 +-
> >  drivers/char/mem.c                            |  86 +-------------
> >  drivers/gpu/drm/exynos/Kconfig                |   1 -
> >  drivers/gpu/drm/exynos/exynos_drm_g2d.c       |  48 ++++----
> >  drivers/media/common/videobuf2/Kconfig        |   1 -
> >  drivers/media/common/videobuf2/Makefile       |   1 +
> >  .../media/common/videobuf2}/frame_vector.c    |  57 ++++-----
> >  .../media/common/videobuf2/videobuf2-memops.c |   3 +-
> >  drivers/media/platform/omap/Kconfig           |   1 -
> >  drivers/media/v4l2-core/videobuf-dma-contig.c |   2 +-
> >  drivers/misc/habanalabs/Kconfig               |   1 -
> >  drivers/misc/habanalabs/common/habanalabs.h   |   6 +-
> >  drivers/misc/habanalabs/common/memory.c       |  52 +++-----
> >  drivers/pci/pci-sysfs.c                       |   4 +
> >  drivers/pci/proc.c                            |   6 +
> >  drivers/vfio/vfio_iommu_type1.c               |   4 +-
> >  fs/sysfs/file.c                               |  11 ++
> >  include/linux/ioport.h                        |   6 +-
> >  include/linux/kvm_host.h                      |   9 +-
> >  include/linux/mm.h                            |  50 +-------
> >  include/linux/sysfs.h                         |   2 +
> >  include/media/frame_vector.h                  |  47 ++++++++
> >  include/media/videobuf2-core.h                |   1 +
> >  kernel/resource.c                             |  98 ++++++++++++++-
> >  mm/Kconfig                                    |   3 -
> >  mm/Makefile                                   |   1 -
> >  mm/memory.c                                   | 112 +++++++++++++++---
> >  mm/nommu.c                                    |  16 ++-
> >  security/Kconfig                              |  13 ++
> >  virt/kvm/kvm_main.c                           |  56 +++++----
> >  33 files changed, 413 insertions(+), 299 deletions(-)
> >  rename {mm => drivers/media/common/videobuf2}/frame_vector.c (84%)
> >  create mode 100644 include/media/frame_vector.h
> >
> > --
> > 2.29.2
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch



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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v7 00/17] follow_pfn and other iomap races
  2021-01-12 13:24 ` [PATCH v7 00/17] follow_pfn and other iomap races Daniel Vetter
  2021-01-12 13:28   ` Daniel Vetter
@ 2021-01-12 20:57   ` Stephen Rothwell
  1 sibling, 0 replies; 32+ messages in thread
From: Stephen Rothwell @ 2021-01-12 20:57 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: DRI Development, LKML, linux-samsung-soc, kvm, Daniel Vetter,
	linux-mm, linux-arm-kernel, linux-media

[-- Attachment #1: Type: text/plain, Size: 1213 bytes --]

Hi Daniel,

On Tue, 12 Jan 2021 14:24:27 +0100 Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> As Jason suggested, I've pulled the first 1 patches into a topic branch.
> 
> Stephen, can you please add the below to linux-next for the 5.12 merge
> window?
> 
> git://anongit.freedesktop.org/drm/drm topic/iomem-mmap-vs-gup

Added from today.

Thanks for adding your subsystem tree as a participant of linux-next.  As
you may know, this is not a judgement of your code.  The purpose of
linux-next is for integration testing and to lower the impact of
conflicts between subsystems in the next merge window. 

You will need to ensure that the patches/commits in your tree/series have
been:
     * submitted under GPL v2 (or later) and include the Contributor's
        Signed-off-by,
     * posted to the relevant mailing list,
     * reviewed by you (or another maintainer of your subsystem tree),
     * successfully unit tested, and 
     * destined for the current or next Linux merge window.

Basically, this should be just what you would send to Linus (or ask him
to fetch).  It is allowed to be rebased if you deem it necessary.

-- 
Cheers,
Stephen Rothwell 
sfr@canb.auug.org.au

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v7 12/17] PCI: Revoke mappings like devmem
  2020-11-27 16:41 ` [PATCH v7 12/17] PCI: Revoke mappings like devmem Daniel Vetter
@ 2021-01-19  8:17   ` Daniel Vetter
  2021-01-19 14:32     ` Greg Kroah-Hartman
  0 siblings, 1 reply; 32+ messages in thread
From: Daniel Vetter @ 2021-01-19  8:17 UTC (permalink / raw)
  To: DRI Development, LKML, Stephen Rothwell
  Cc: KVM list, Linux MM, Linux ARM, linux-samsung-soc,
	open list:DMA BUFFER SHARING FRAMEWORK, Bjorn Helgaas,
	Dan Williams, Daniel Vetter, Jason Gunthorpe, Kees Cook,
	Andrew Morton, John Hubbard, Jérôme Glisse, Jan Kara,
	Greg Kroah-Hartman, Linux PCI

On Fri, Nov 27, 2020 at 5:42 PM Daniel Vetter <daniel.vetter@ffwll.ch> 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.
>
> 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. The cleanest way is
> to adjust this at at ->open time:
>
> - for sysfs this is easy, now that binary attributes support this. We
>   just set bin_attr->mapping when mmap is supported
> - for procfs it's a bit more tricky, since procfs pci access has only
>   one file per device, and access to a specific resources first needs
>   to be set up with some ioctl calls. But mmap is only supported for
>   the same resources as sysfs exposes with mmap support, and otherwise
>   rejected, so we can set the mapping unconditionally at open time
>   without harm.
>
> A special consideration is for arch_can_pci_mmap_io() - we need to
> make sure that the ->f_mapping doesn't alias between ioport and iomem
> space. There's only 2 ways in-tree to support mmap of ioports: generic
> pci mmap (ARCH_GENERIC_PCI_MMAP_RESOURCE), and sparc as the single
> architecture hand-rolling. Both approach support ioport mmap through a
> special pfn range and not through magic pte attributes. Aliasing is
> therefore not a problem.
>
> The only difference in access checks left is that sysfs PCI mmap does
> not check for CAP_RAWIO. I'm not really sure whether that should be
> added or not.
>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> 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: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 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: Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-pci@vger.kernel.org
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> --
> v2:
> - Totally new approach: Adjust filp->f_mapping at open time. Note that
>   this now works on all architectures, not just those support
>   ARCH_GENERIC_PCI_MMAP_RESOURCE
> ---
>  drivers/pci/pci-sysfs.c | 4 ++++
>  drivers/pci/proc.c      | 1 +
>  2 files changed, 5 insertions(+)
>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index d15c881e2e7e..3f1c31bc0b7c 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -929,6 +929,7 @@ void pci_create_legacy_files(struct pci_bus *b)
>         b->legacy_io->read = pci_read_legacy_io;
>         b->legacy_io->write = pci_write_legacy_io;
>         b->legacy_io->mmap = pci_mmap_legacy_io;
> +       b->legacy_io->mapping = iomem_get_mapping();
>         pci_adjust_legacy_attr(b, pci_mmap_io);
>         error = device_create_bin_file(&b->dev, b->legacy_io);
>         if (error)
> @@ -941,6 +942,7 @@ void pci_create_legacy_files(struct pci_bus *b)
>         b->legacy_mem->size = 1024*1024;
>         b->legacy_mem->attr.mode = 0600;
>         b->legacy_mem->mmap = pci_mmap_legacy_mem;
> +       b->legacy_io->mapping = iomem_get_mapping();

Unlike the normal pci stuff below, the legacy files here go boom
because they're set up much earlier in the boot sequence. This only
affects HAVE_PCI_LEGACY architectures, which aren't that many. So what
should we do here now:
- drop the devmem revoke for these
- rework the init sequence somehow to set up these files a lot later
- redo the sysfs patch so that it doesn't take an address_space
pointer, but instead a callback to get at that (since at open time
everything is set up). Imo rather ugly
- ditch this part of the series (since there's not really any takers
for the latter parts it might just not make sense to push for this)
- something else?

Bjorn, Greg, thoughts?

Issuge got reported by Stephen on a powerpc when trying to build
linux-next with this patch included.

Thanks, Daniel

>         pci_adjust_legacy_attr(b, pci_mmap_mem);
>         error = device_create_bin_file(&b->dev, b->legacy_mem);
>         if (error)
> @@ -1156,6 +1158,8 @@ static int pci_create_attr(struct pci_dev *pdev, int num, int write_combine)
>                         res_attr->mmap = pci_mmap_resource_uc;
>                 }
>         }
> +       if (res_attr->mmap)
> +               res_attr->mapping = iomem_get_mapping();
>         res_attr->attr.name = res_attr_name;
>         res_attr->attr.mode = 0600;
>         res_attr->size = pci_resource_len(pdev, num);
> diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
> index 3a2f90beb4cb..9bab07302bbf 100644
> --- a/drivers/pci/proc.c
> +++ b/drivers/pci/proc.c
> @@ -298,6 +298,7 @@ static int proc_bus_pci_open(struct inode *inode, struct file *file)
>         fpriv->write_combine = 0;
>
>         file->private_data = fpriv;
> +       file->f_mapping = iomem_get_mapping();
>
>         return 0;
>  }
> --
> 2.29.2
>


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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v7 14/17] media/videobuf1|2: Mark follow_pfn usage as unsafe
  2020-11-27 16:41 ` [PATCH v7 14/17] media/videobuf1|2: Mark follow_pfn usage as unsafe Daniel Vetter
@ 2021-01-19 12:24   ` Hans Verkuil
  0 siblings, 0 replies; 32+ messages in thread
From: Hans Verkuil @ 2021-01-19 12:24 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development, LKML
  Cc: kvm, linux-mm, linux-arm-kernel, linux-samsung-soc, linux-media,
	Tomasz Figa, Daniel Vetter, Jason Gunthorpe, Kees Cook,
	Dan Williams, Andrew Morton, John Hubbard,
	Jérôme Glisse, Jan Kara, Pawel Osciak,
	Marek Szyprowski, Kyungmin Park, Laurent Dufour, Vlastimil Babka,
	Daniel Jordan, Michel Lespinasse

On 27/11/2020 17:41, Daniel Vetter wrote:
> The media model assumes that buffers are all preallocated, so that
> when a media pipeline is running we never miss a deadline because the
> buffers aren't allocated or available.
> 
> This means we cannot fix the v4l follow_pfn usage through
> mmu_notifier, without breaking how this all works. The only real fix
> is to deprecate userptr support for VM_IO | VM_PFNMAP mappings and
> tell everyone to cut over to dma-buf memory sharing for zerocopy.
> 
> userptr for normal memory will keep working as-is, this only affects
> the zerocopy userptr usage enabled in 50ac952d2263 ("[media]
> videobuf2-dma-sg: Support io userptr operations on io memory").
> 
> Acked-by: Tomasz Figa <tfiga@chromium.org>

Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Regards,

	Hans

> 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: 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: Laurent Dufour <ldufour@linux.ibm.com>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
> Cc: Michel Lespinasse <walken@google.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> --
> v3:
> - Reference the commit that enabled the zerocopy userptr use case to
>   make it abundandtly clear that this patch only affects that, and not
>   normal memory userptr. The old commit message already explained that
>   normal memory userptr is unaffected, but I guess that was not clear
>   enough.
> ---
>  drivers/media/common/videobuf2/frame_vector.c | 2 +-
>  drivers/media/v4l2-core/videobuf-dma-contig.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/frame_vector.c b/drivers/media/common/videobuf2/frame_vector.c
> index a0e65481a201..1a82ec13ea00 100644
> --- a/drivers/media/common/videobuf2/frame_vector.c
> +++ b/drivers/media/common/videobuf2/frame_vector.c
> @@ -70,7 +70,7 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames,
>  			break;
>  
>  		while (ret < nr_frames && start + PAGE_SIZE <= vma->vm_end) {
> -			err = follow_pfn(vma, start, &nums[ret]);
> +			err = unsafe_follow_pfn(vma, start, &nums[ret]);
>  			if (err) {
>  				if (ret == 0)
>  					ret = err;
> diff --git a/drivers/media/v4l2-core/videobuf-dma-contig.c b/drivers/media/v4l2-core/videobuf-dma-contig.c
> index 52312ce2ba05..821c4a76ab96 100644
> --- a/drivers/media/v4l2-core/videobuf-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf-dma-contig.c
> @@ -183,7 +183,7 @@ static int videobuf_dma_contig_user_get(struct videobuf_dma_contig_memory *mem,
>  	user_address = untagged_baddr;
>  
>  	while (pages_done < (mem->size >> PAGE_SHIFT)) {
> -		ret = follow_pfn(vma, user_address, &this_pfn);
> +		ret = unsafe_follow_pfn(vma, user_address, &this_pfn);
>  		if (ret)
>  			break;
>  
> 


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v7 12/17] PCI: Revoke mappings like devmem
  2021-01-19  8:17   ` Daniel Vetter
@ 2021-01-19 14:32     ` Greg Kroah-Hartman
  2021-01-19 14:34       ` Daniel Vetter
  0 siblings, 1 reply; 32+ messages in thread
From: Greg Kroah-Hartman @ 2021-01-19 14:32 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: DRI Development, LKML, Stephen Rothwell, KVM list, Linux MM,
	Linux ARM, linux-samsung-soc,
	open list:DMA BUFFER SHARING FRAMEWORK, Bjorn Helgaas,
	Dan Williams, Daniel Vetter, Jason Gunthorpe, Kees Cook,
	Andrew Morton, John Hubbard, Jérôme Glisse, Jan Kara,
	Linux PCI

On Tue, Jan 19, 2021 at 09:17:55AM +0100, Daniel Vetter wrote:
> On Fri, Nov 27, 2020 at 5:42 PM Daniel Vetter <daniel.vetter@ffwll.ch> 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.
> >
> > 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. The cleanest way is
> > to adjust this at at ->open time:
> >
> > - for sysfs this is easy, now that binary attributes support this. We
> >   just set bin_attr->mapping when mmap is supported
> > - for procfs it's a bit more tricky, since procfs pci access has only
> >   one file per device, and access to a specific resources first needs
> >   to be set up with some ioctl calls. But mmap is only supported for
> >   the same resources as sysfs exposes with mmap support, and otherwise
> >   rejected, so we can set the mapping unconditionally at open time
> >   without harm.
> >
> > A special consideration is for arch_can_pci_mmap_io() - we need to
> > make sure that the ->f_mapping doesn't alias between ioport and iomem
> > space. There's only 2 ways in-tree to support mmap of ioports: generic
> > pci mmap (ARCH_GENERIC_PCI_MMAP_RESOURCE), and sparc as the single
> > architecture hand-rolling. Both approach support ioport mmap through a
> > special pfn range and not through magic pte attributes. Aliasing is
> > therefore not a problem.
> >
> > The only difference in access checks left is that sysfs PCI mmap does
> > not check for CAP_RAWIO. I'm not really sure whether that should be
> > added or not.
> >
> > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > 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: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > 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: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: linux-pci@vger.kernel.org
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > --
> > v2:
> > - Totally new approach: Adjust filp->f_mapping at open time. Note that
> >   this now works on all architectures, not just those support
> >   ARCH_GENERIC_PCI_MMAP_RESOURCE
> > ---
> >  drivers/pci/pci-sysfs.c | 4 ++++
> >  drivers/pci/proc.c      | 1 +
> >  2 files changed, 5 insertions(+)
> >
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index d15c881e2e7e..3f1c31bc0b7c 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -929,6 +929,7 @@ void pci_create_legacy_files(struct pci_bus *b)
> >         b->legacy_io->read = pci_read_legacy_io;
> >         b->legacy_io->write = pci_write_legacy_io;
> >         b->legacy_io->mmap = pci_mmap_legacy_io;
> > +       b->legacy_io->mapping = iomem_get_mapping();
> >         pci_adjust_legacy_attr(b, pci_mmap_io);
> >         error = device_create_bin_file(&b->dev, b->legacy_io);
> >         if (error)
> > @@ -941,6 +942,7 @@ void pci_create_legacy_files(struct pci_bus *b)
> >         b->legacy_mem->size = 1024*1024;
> >         b->legacy_mem->attr.mode = 0600;
> >         b->legacy_mem->mmap = pci_mmap_legacy_mem;
> > +       b->legacy_io->mapping = iomem_get_mapping();
> 
> Unlike the normal pci stuff below, the legacy files here go boom
> because they're set up much earlier in the boot sequence. This only
> affects HAVE_PCI_LEGACY architectures, which aren't that many. So what
> should we do here now:
> - drop the devmem revoke for these
> - rework the init sequence somehow to set up these files a lot later
> - redo the sysfs patch so that it doesn't take an address_space
> pointer, but instead a callback to get at that (since at open time
> everything is set up). Imo rather ugly
> - ditch this part of the series (since there's not really any takers
> for the latter parts it might just not make sense to push for this)
> - something else?
> 
> Bjorn, Greg, thoughts?

What sysfs patch are you referring to here?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v7 12/17] PCI: Revoke mappings like devmem
  2021-01-19 14:32     ` Greg Kroah-Hartman
@ 2021-01-19 14:34       ` Daniel Vetter
  2021-01-19 15:20         ` Greg Kroah-Hartman
  0 siblings, 1 reply; 32+ messages in thread
From: Daniel Vetter @ 2021-01-19 14:34 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: DRI Development, LKML, Stephen Rothwell, KVM list, Linux MM,
	Linux ARM, linux-samsung-soc,
	open list:DMA BUFFER SHARING FRAMEWORK, Bjorn Helgaas,
	Dan Williams, Daniel Vetter, Jason Gunthorpe, Kees Cook,
	Andrew Morton, John Hubbard, Jérôme Glisse, Jan Kara,
	Linux PCI

On Tue, Jan 19, 2021 at 3:32 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Jan 19, 2021 at 09:17:55AM +0100, Daniel Vetter wrote:
> > On Fri, Nov 27, 2020 at 5:42 PM Daniel Vetter <daniel.vetter@ffwll.ch> 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.
> > >
> > > 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. The cleanest way is
> > > to adjust this at at ->open time:
> > >
> > > - for sysfs this is easy, now that binary attributes support this. We
> > >   just set bin_attr->mapping when mmap is supported
> > > - for procfs it's a bit more tricky, since procfs pci access has only
> > >   one file per device, and access to a specific resources first needs
> > >   to be set up with some ioctl calls. But mmap is only supported for
> > >   the same resources as sysfs exposes with mmap support, and otherwise
> > >   rejected, so we can set the mapping unconditionally at open time
> > >   without harm.
> > >
> > > A special consideration is for arch_can_pci_mmap_io() - we need to
> > > make sure that the ->f_mapping doesn't alias between ioport and iomem
> > > space. There's only 2 ways in-tree to support mmap of ioports: generic
> > > pci mmap (ARCH_GENERIC_PCI_MMAP_RESOURCE), and sparc as the single
> > > architecture hand-rolling. Both approach support ioport mmap through a
> > > special pfn range and not through magic pte attributes. Aliasing is
> > > therefore not a problem.
> > >
> > > The only difference in access checks left is that sysfs PCI mmap does
> > > not check for CAP_RAWIO. I'm not really sure whether that should be
> > > added or not.
> > >
> > > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > > 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: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > 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: Bjorn Helgaas <bhelgaas@google.com>
> > > Cc: linux-pci@vger.kernel.org
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > --
> > > v2:
> > > - Totally new approach: Adjust filp->f_mapping at open time. Note that
> > >   this now works on all architectures, not just those support
> > >   ARCH_GENERIC_PCI_MMAP_RESOURCE
> > > ---
> > >  drivers/pci/pci-sysfs.c | 4 ++++
> > >  drivers/pci/proc.c      | 1 +
> > >  2 files changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > > index d15c881e2e7e..3f1c31bc0b7c 100644
> > > --- a/drivers/pci/pci-sysfs.c
> > > +++ b/drivers/pci/pci-sysfs.c
> > > @@ -929,6 +929,7 @@ void pci_create_legacy_files(struct pci_bus *b)
> > >         b->legacy_io->read = pci_read_legacy_io;
> > >         b->legacy_io->write = pci_write_legacy_io;
> > >         b->legacy_io->mmap = pci_mmap_legacy_io;
> > > +       b->legacy_io->mapping = iomem_get_mapping();
> > >         pci_adjust_legacy_attr(b, pci_mmap_io);
> > >         error = device_create_bin_file(&b->dev, b->legacy_io);
> > >         if (error)
> > > @@ -941,6 +942,7 @@ void pci_create_legacy_files(struct pci_bus *b)
> > >         b->legacy_mem->size = 1024*1024;
> > >         b->legacy_mem->attr.mode = 0600;
> > >         b->legacy_mem->mmap = pci_mmap_legacy_mem;
> > > +       b->legacy_io->mapping = iomem_get_mapping();
> >
> > Unlike the normal pci stuff below, the legacy files here go boom
> > because they're set up much earlier in the boot sequence. This only
> > affects HAVE_PCI_LEGACY architectures, which aren't that many. So what
> > should we do here now:
> > - drop the devmem revoke for these
> > - rework the init sequence somehow to set up these files a lot later
> > - redo the sysfs patch so that it doesn't take an address_space
> > pointer, but instead a callback to get at that (since at open time
> > everything is set up). Imo rather ugly
> > - ditch this part of the series (since there's not really any takers
> > for the latter parts it might just not make sense to push for this)
> > - something else?
> >
> > Bjorn, Greg, thoughts?
>
> What sysfs patch are you referring to here?

Currently in linux-next:

commit 74b30195395c406c787280a77ae55aed82dbbfc7 (HEAD ->
topic/iomem-mmap-vs-gup, drm/topic/iomem-mmap-vs-gup)
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Fri Nov 27 17:41:25 2020 +0100

   sysfs: Support zapping of binary attr mmaps

Or the patch right before this one in this submission here:

https://lore.kernel.org/dri-devel/20201127164131.2244124-12-daniel.vetter@ffwll.ch/

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v7 12/17] PCI: Revoke mappings like devmem
  2021-01-19 14:34       ` Daniel Vetter
@ 2021-01-19 15:20         ` Greg Kroah-Hartman
  2021-01-19 16:03           ` Daniel Vetter
  0 siblings, 1 reply; 32+ messages in thread
From: Greg Kroah-Hartman @ 2021-01-19 15:20 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: DRI Development, LKML, Stephen Rothwell, KVM list, Linux MM,
	Linux ARM, linux-samsung-soc,
	open list:DMA BUFFER SHARING FRAMEWORK, Bjorn Helgaas,
	Dan Williams, Daniel Vetter, Jason Gunthorpe, Kees Cook,
	Andrew Morton, John Hubbard, Jérôme Glisse, Jan Kara,
	Linux PCI

On Tue, Jan 19, 2021 at 03:34:47PM +0100, Daniel Vetter wrote:
> On Tue, Jan 19, 2021 at 3:32 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Jan 19, 2021 at 09:17:55AM +0100, Daniel Vetter wrote:
> > > On Fri, Nov 27, 2020 at 5:42 PM Daniel Vetter <daniel.vetter@ffwll.ch> 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.
> > > >
> > > > 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. The cleanest way is
> > > > to adjust this at at ->open time:
> > > >
> > > > - for sysfs this is easy, now that binary attributes support this. We
> > > >   just set bin_attr->mapping when mmap is supported
> > > > - for procfs it's a bit more tricky, since procfs pci access has only
> > > >   one file per device, and access to a specific resources first needs
> > > >   to be set up with some ioctl calls. But mmap is only supported for
> > > >   the same resources as sysfs exposes with mmap support, and otherwise
> > > >   rejected, so we can set the mapping unconditionally at open time
> > > >   without harm.
> > > >
> > > > A special consideration is for arch_can_pci_mmap_io() - we need to
> > > > make sure that the ->f_mapping doesn't alias between ioport and iomem
> > > > space. There's only 2 ways in-tree to support mmap of ioports: generic
> > > > pci mmap (ARCH_GENERIC_PCI_MMAP_RESOURCE), and sparc as the single
> > > > architecture hand-rolling. Both approach support ioport mmap through a
> > > > special pfn range and not through magic pte attributes. Aliasing is
> > > > therefore not a problem.
> > > >
> > > > The only difference in access checks left is that sysfs PCI mmap does
> > > > not check for CAP_RAWIO. I'm not really sure whether that should be
> > > > added or not.
> > > >
> > > > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > > > 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: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > 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: Bjorn Helgaas <bhelgaas@google.com>
> > > > Cc: linux-pci@vger.kernel.org
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > --
> > > > v2:
> > > > - Totally new approach: Adjust filp->f_mapping at open time. Note that
> > > >   this now works on all architectures, not just those support
> > > >   ARCH_GENERIC_PCI_MMAP_RESOURCE
> > > > ---
> > > >  drivers/pci/pci-sysfs.c | 4 ++++
> > > >  drivers/pci/proc.c      | 1 +
> > > >  2 files changed, 5 insertions(+)
> > > >
> > > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > > > index d15c881e2e7e..3f1c31bc0b7c 100644
> > > > --- a/drivers/pci/pci-sysfs.c
> > > > +++ b/drivers/pci/pci-sysfs.c
> > > > @@ -929,6 +929,7 @@ void pci_create_legacy_files(struct pci_bus *b)
> > > >         b->legacy_io->read = pci_read_legacy_io;
> > > >         b->legacy_io->write = pci_write_legacy_io;
> > > >         b->legacy_io->mmap = pci_mmap_legacy_io;
> > > > +       b->legacy_io->mapping = iomem_get_mapping();
> > > >         pci_adjust_legacy_attr(b, pci_mmap_io);
> > > >         error = device_create_bin_file(&b->dev, b->legacy_io);
> > > >         if (error)
> > > > @@ -941,6 +942,7 @@ void pci_create_legacy_files(struct pci_bus *b)
> > > >         b->legacy_mem->size = 1024*1024;
> > > >         b->legacy_mem->attr.mode = 0600;
> > > >         b->legacy_mem->mmap = pci_mmap_legacy_mem;
> > > > +       b->legacy_io->mapping = iomem_get_mapping();
> > >
> > > Unlike the normal pci stuff below, the legacy files here go boom
> > > because they're set up much earlier in the boot sequence. This only
> > > affects HAVE_PCI_LEGACY architectures, which aren't that many. So what
> > > should we do here now:
> > > - drop the devmem revoke for these
> > > - rework the init sequence somehow to set up these files a lot later
> > > - redo the sysfs patch so that it doesn't take an address_space
> > > pointer, but instead a callback to get at that (since at open time
> > > everything is set up). Imo rather ugly
> > > - ditch this part of the series (since there's not really any takers
> > > for the latter parts it might just not make sense to push for this)
> > > - something else?
> > >
> > > Bjorn, Greg, thoughts?
> >
> > What sysfs patch are you referring to here?
> 
> Currently in linux-next:
> 
> commit 74b30195395c406c787280a77ae55aed82dbbfc7 (HEAD ->
> topic/iomem-mmap-vs-gup, drm/topic/iomem-mmap-vs-gup)
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Fri Nov 27 17:41:25 2020 +0100
> 
>    sysfs: Support zapping of binary attr mmaps
> 
> Or the patch right before this one in this submission here:
> 
> https://lore.kernel.org/dri-devel/20201127164131.2244124-12-daniel.vetter@ffwll.ch/

Ah.  Hm, a callback in the sysfs file logic seems really hairy, so I
would prefer that not happen.  If no one really needs this stuff, why
not just drop it like you mention?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v7 12/17] PCI: Revoke mappings like devmem
  2021-01-19 15:20         ` Greg Kroah-Hartman
@ 2021-01-19 16:03           ` Daniel Vetter
  2021-02-03 16:14             ` Daniel Vetter
  0 siblings, 1 reply; 32+ messages in thread
From: Daniel Vetter @ 2021-01-19 16:03 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: DRI Development, LKML, Stephen Rothwell, KVM list, Linux MM,
	Linux ARM, linux-samsung-soc,
	open list:DMA BUFFER SHARING FRAMEWORK, Bjorn Helgaas,
	Dan Williams, Daniel Vetter, Jason Gunthorpe, Kees Cook,
	Andrew Morton, John Hubbard, Jérôme Glisse, Jan Kara,
	Linux PCI

On Tue, Jan 19, 2021 at 4:20 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Jan 19, 2021 at 03:34:47PM +0100, Daniel Vetter wrote:
> > On Tue, Jan 19, 2021 at 3:32 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Tue, Jan 19, 2021 at 09:17:55AM +0100, Daniel Vetter wrote:
> > > > On Fri, Nov 27, 2020 at 5:42 PM Daniel Vetter <daniel.vetter@ffwll.ch> 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.
> > > > >
> > > > > 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. The cleanest way is
> > > > > to adjust this at at ->open time:
> > > > >
> > > > > - for sysfs this is easy, now that binary attributes support this. We
> > > > >   just set bin_attr->mapping when mmap is supported
> > > > > - for procfs it's a bit more tricky, since procfs pci access has only
> > > > >   one file per device, and access to a specific resources first needs
> > > > >   to be set up with some ioctl calls. But mmap is only supported for
> > > > >   the same resources as sysfs exposes with mmap support, and otherwise
> > > > >   rejected, so we can set the mapping unconditionally at open time
> > > > >   without harm.
> > > > >
> > > > > A special consideration is for arch_can_pci_mmap_io() - we need to
> > > > > make sure that the ->f_mapping doesn't alias between ioport and iomem
> > > > > space. There's only 2 ways in-tree to support mmap of ioports: generic
> > > > > pci mmap (ARCH_GENERIC_PCI_MMAP_RESOURCE), and sparc as the single
> > > > > architecture hand-rolling. Both approach support ioport mmap through a
> > > > > special pfn range and not through magic pte attributes. Aliasing is
> > > > > therefore not a problem.
> > > > >
> > > > > The only difference in access checks left is that sysfs PCI mmap does
> > > > > not check for CAP_RAWIO. I'm not really sure whether that should be
> > > > > added or not.
> > > > >
> > > > > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> > > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > > > > 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: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > 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: Bjorn Helgaas <bhelgaas@google.com>
> > > > > Cc: linux-pci@vger.kernel.org
> > > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > --
> > > > > v2:
> > > > > - Totally new approach: Adjust filp->f_mapping at open time. Note that
> > > > >   this now works on all architectures, not just those support
> > > > >   ARCH_GENERIC_PCI_MMAP_RESOURCE
> > > > > ---
> > > > >  drivers/pci/pci-sysfs.c | 4 ++++
> > > > >  drivers/pci/proc.c      | 1 +
> > > > >  2 files changed, 5 insertions(+)
> > > > >
> > > > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > > > > index d15c881e2e7e..3f1c31bc0b7c 100644
> > > > > --- a/drivers/pci/pci-sysfs.c
> > > > > +++ b/drivers/pci/pci-sysfs.c
> > > > > @@ -929,6 +929,7 @@ void pci_create_legacy_files(struct pci_bus *b)
> > > > >         b->legacy_io->read = pci_read_legacy_io;
> > > > >         b->legacy_io->write = pci_write_legacy_io;
> > > > >         b->legacy_io->mmap = pci_mmap_legacy_io;
> > > > > +       b->legacy_io->mapping = iomem_get_mapping();
> > > > >         pci_adjust_legacy_attr(b, pci_mmap_io);
> > > > >         error = device_create_bin_file(&b->dev, b->legacy_io);
> > > > >         if (error)
> > > > > @@ -941,6 +942,7 @@ void pci_create_legacy_files(struct pci_bus *b)
> > > > >         b->legacy_mem->size = 1024*1024;
> > > > >         b->legacy_mem->attr.mode = 0600;
> > > > >         b->legacy_mem->mmap = pci_mmap_legacy_mem;
> > > > > +       b->legacy_io->mapping = iomem_get_mapping();
> > > >
> > > > Unlike the normal pci stuff below, the legacy files here go boom
> > > > because they're set up much earlier in the boot sequence. This only
> > > > affects HAVE_PCI_LEGACY architectures, which aren't that many. So what
> > > > should we do here now:
> > > > - drop the devmem revoke for these
> > > > - rework the init sequence somehow to set up these files a lot later
> > > > - redo the sysfs patch so that it doesn't take an address_space
> > > > pointer, but instead a callback to get at that (since at open time
> > > > everything is set up). Imo rather ugly
> > > > - ditch this part of the series (since there's not really any takers
> > > > for the latter parts it might just not make sense to push for this)
> > > > - something else?
> > > >
> > > > Bjorn, Greg, thoughts?
> > >
> > > What sysfs patch are you referring to here?
> >
> > Currently in linux-next:
> >
> > commit 74b30195395c406c787280a77ae55aed82dbbfc7 (HEAD ->
> > topic/iomem-mmap-vs-gup, drm/topic/iomem-mmap-vs-gup)
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date:   Fri Nov 27 17:41:25 2020 +0100
> >
> >    sysfs: Support zapping of binary attr mmaps
> >
> > Or the patch right before this one in this submission here:
> >
> > https://lore.kernel.org/dri-devel/20201127164131.2244124-12-daniel.vetter@ffwll.ch/
>
> Ah.  Hm, a callback in the sysfs file logic seems really hairy, so I
> would prefer that not happen.  If no one really needs this stuff, why
> not just drop it like you mention?

Well it is needed, but just on architectures I don't care about much.
Most relevant is perhaps powerpc (that's where Stephen hit the issue).
I do wonder whether we could move the legacy pci files setup to where
the modern stuff is set up from pci_create_resource_files() or maybe
pci_create_sysfs_dev_files() even for HAVE_PCI_LEGACY. I think that
might work, but since it's legacy flow on some funny architectures
(alpha, itanium, that kind of stuff) I have no idea what kind of
monsters I'm going to anger :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v7 12/17] PCI: Revoke mappings like devmem
  2021-01-19 16:03           ` Daniel Vetter
@ 2021-02-03 16:14             ` Daniel Vetter
  2021-02-04 10:23               ` Daniel Vetter
  0 siblings, 1 reply; 32+ messages in thread
From: Daniel Vetter @ 2021-02-03 16:14 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: DRI Development, LKML, Stephen Rothwell, KVM list, Linux MM,
	Linux ARM, linux-samsung-soc,
	open list:DMA BUFFER SHARING FRAMEWORK, Bjorn Helgaas,
	Dan Williams, Daniel Vetter, Jason Gunthorpe, Kees Cook,
	Andrew Morton, John Hubbard, Jérôme Glisse, Jan Kara,
	Linux PCI

On Tue, Jan 19, 2021 at 5:03 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> On Tue, Jan 19, 2021 at 4:20 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Jan 19, 2021 at 03:34:47PM +0100, Daniel Vetter wrote:
> > > On Tue, Jan 19, 2021 at 3:32 PM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Tue, Jan 19, 2021 at 09:17:55AM +0100, Daniel Vetter wrote:
> > > > > On Fri, Nov 27, 2020 at 5:42 PM Daniel Vetter <daniel.vetter@ffwll.ch> 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.
> > > > > >
> > > > > > 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. The cleanest way is
> > > > > > to adjust this at at ->open time:
> > > > > >
> > > > > > - for sysfs this is easy, now that binary attributes support this. We
> > > > > >   just set bin_attr->mapping when mmap is supported
> > > > > > - for procfs it's a bit more tricky, since procfs pci access has only
> > > > > >   one file per device, and access to a specific resources first needs
> > > > > >   to be set up with some ioctl calls. But mmap is only supported for
> > > > > >   the same resources as sysfs exposes with mmap support, and otherwise
> > > > > >   rejected, so we can set the mapping unconditionally at open time
> > > > > >   without harm.
> > > > > >
> > > > > > A special consideration is for arch_can_pci_mmap_io() - we need to
> > > > > > make sure that the ->f_mapping doesn't alias between ioport and iomem
> > > > > > space. There's only 2 ways in-tree to support mmap of ioports: generic
> > > > > > pci mmap (ARCH_GENERIC_PCI_MMAP_RESOURCE), and sparc as the single
> > > > > > architecture hand-rolling. Both approach support ioport mmap through a
> > > > > > special pfn range and not through magic pte attributes. Aliasing is
> > > > > > therefore not a problem.
> > > > > >
> > > > > > The only difference in access checks left is that sysfs PCI mmap does
> > > > > > not check for CAP_RAWIO. I'm not really sure whether that should be
> > > > > > added or not.
> > > > > >
> > > > > > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> > > > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > > > > > 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: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > > 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: Bjorn Helgaas <bhelgaas@google.com>
> > > > > > Cc: linux-pci@vger.kernel.org
> > > > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > > --
> > > > > > v2:
> > > > > > - Totally new approach: Adjust filp->f_mapping at open time. Note that
> > > > > >   this now works on all architectures, not just those support
> > > > > >   ARCH_GENERIC_PCI_MMAP_RESOURCE
> > > > > > ---
> > > > > >  drivers/pci/pci-sysfs.c | 4 ++++
> > > > > >  drivers/pci/proc.c      | 1 +
> > > > > >  2 files changed, 5 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > > > > > index d15c881e2e7e..3f1c31bc0b7c 100644
> > > > > > --- a/drivers/pci/pci-sysfs.c
> > > > > > +++ b/drivers/pci/pci-sysfs.c
> > > > > > @@ -929,6 +929,7 @@ void pci_create_legacy_files(struct pci_bus *b)
> > > > > >         b->legacy_io->read = pci_read_legacy_io;
> > > > > >         b->legacy_io->write = pci_write_legacy_io;
> > > > > >         b->legacy_io->mmap = pci_mmap_legacy_io;
> > > > > > +       b->legacy_io->mapping = iomem_get_mapping();
> > > > > >         pci_adjust_legacy_attr(b, pci_mmap_io);
> > > > > >         error = device_create_bin_file(&b->dev, b->legacy_io);
> > > > > >         if (error)
> > > > > > @@ -941,6 +942,7 @@ void pci_create_legacy_files(struct pci_bus *b)
> > > > > >         b->legacy_mem->size = 1024*1024;
> > > > > >         b->legacy_mem->attr.mode = 0600;
> > > > > >         b->legacy_mem->mmap = pci_mmap_legacy_mem;
> > > > > > +       b->legacy_io->mapping = iomem_get_mapping();
> > > > >
> > > > > Unlike the normal pci stuff below, the legacy files here go boom
> > > > > because they're set up much earlier in the boot sequence. This only
> > > > > affects HAVE_PCI_LEGACY architectures, which aren't that many. So what
> > > > > should we do here now:
> > > > > - drop the devmem revoke for these
> > > > > - rework the init sequence somehow to set up these files a lot later
> > > > > - redo the sysfs patch so that it doesn't take an address_space
> > > > > pointer, but instead a callback to get at that (since at open time
> > > > > everything is set up). Imo rather ugly
> > > > > - ditch this part of the series (since there's not really any takers
> > > > > for the latter parts it might just not make sense to push for this)
> > > > > - something else?
> > > > >
> > > > > Bjorn, Greg, thoughts?
> > > >
> > > > What sysfs patch are you referring to here?
> > >
> > > Currently in linux-next:
> > >
> > > commit 74b30195395c406c787280a77ae55aed82dbbfc7 (HEAD ->
> > > topic/iomem-mmap-vs-gup, drm/topic/iomem-mmap-vs-gup)
> > > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Date:   Fri Nov 27 17:41:25 2020 +0100
> > >
> > >    sysfs: Support zapping of binary attr mmaps
> > >
> > > Or the patch right before this one in this submission here:
> > >
> > > https://lore.kernel.org/dri-devel/20201127164131.2244124-12-daniel.vetter@ffwll.ch/
> >
> > Ah.  Hm, a callback in the sysfs file logic seems really hairy, so I
> > would prefer that not happen.  If no one really needs this stuff, why
> > not just drop it like you mention?
>
> Well it is needed, but just on architectures I don't care about much.
> Most relevant is perhaps powerpc (that's where Stephen hit the issue).
> I do wonder whether we could move the legacy pci files setup to where
> the modern stuff is set up from pci_create_resource_files() or maybe
> pci_create_sysfs_dev_files() even for HAVE_PCI_LEGACY. I think that
> might work, but since it's legacy flow on some funny architectures
> (alpha, itanium, that kind of stuff) I have no idea what kind of
> monsters I'm going to anger :-)

Back from a week of vacation, I looked at this again and I think
shouldn't be hard to fix this with the sam trick
pci_create_sysfs_dev_files() uses: As long as sysfs_initialized isn't
set we skip, and then later on when the vfs is up&running we can
initialize everything.

To be able to apply the same thing to pci_create_legacy_files() I
think all I need is to iterate overa all struct pci_bus in
pci_sysfs_init() and we're good. Unfortunately I didn't find any
for_each_pci_bus(), so how do I do that?

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v7 12/17] PCI: Revoke mappings like devmem
  2021-02-03 16:14             ` Daniel Vetter
@ 2021-02-04 10:23               ` Daniel Vetter
  0 siblings, 0 replies; 32+ messages in thread
From: Daniel Vetter @ 2021-02-04 10:23 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: DRI Development, LKML, Stephen Rothwell, KVM list, Linux MM,
	Linux ARM, linux-samsung-soc,
	open list:DMA BUFFER SHARING FRAMEWORK, Bjorn Helgaas,
	Dan Williams, Daniel Vetter, Jason Gunthorpe, Kees Cook,
	Andrew Morton, John Hubbard, Jérôme Glisse, Jan Kara,
	Linux PCI

On Wed, Feb 3, 2021 at 5:14 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> On Tue, Jan 19, 2021 at 5:03 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> > On Tue, Jan 19, 2021 at 4:20 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Tue, Jan 19, 2021 at 03:34:47PM +0100, Daniel Vetter wrote:
> > > > On Tue, Jan 19, 2021 at 3:32 PM Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Tue, Jan 19, 2021 at 09:17:55AM +0100, Daniel Vetter wrote:
> > > > > > On Fri, Nov 27, 2020 at 5:42 PM Daniel Vetter <daniel.vetter@ffwll.ch> 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.
> > > > > > >
> > > > > > > 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. The cleanest way is
> > > > > > > to adjust this at at ->open time:
> > > > > > >
> > > > > > > - for sysfs this is easy, now that binary attributes support this. We
> > > > > > >   just set bin_attr->mapping when mmap is supported
> > > > > > > - for procfs it's a bit more tricky, since procfs pci access has only
> > > > > > >   one file per device, and access to a specific resources first needs
> > > > > > >   to be set up with some ioctl calls. But mmap is only supported for
> > > > > > >   the same resources as sysfs exposes with mmap support, and otherwise
> > > > > > >   rejected, so we can set the mapping unconditionally at open time
> > > > > > >   without harm.
> > > > > > >
> > > > > > > A special consideration is for arch_can_pci_mmap_io() - we need to
> > > > > > > make sure that the ->f_mapping doesn't alias between ioport and iomem
> > > > > > > space. There's only 2 ways in-tree to support mmap of ioports: generic
> > > > > > > pci mmap (ARCH_GENERIC_PCI_MMAP_RESOURCE), and sparc as the single
> > > > > > > architecture hand-rolling. Both approach support ioport mmap through a
> > > > > > > special pfn range and not through magic pte attributes. Aliasing is
> > > > > > > therefore not a problem.
> > > > > > >
> > > > > > > The only difference in access checks left is that sysfs PCI mmap does
> > > > > > > not check for CAP_RAWIO. I'm not really sure whether that should be
> > > > > > > added or not.
> > > > > > >
> > > > > > > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> > > > > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > > > > > > 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: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > > > 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: Bjorn Helgaas <bhelgaas@google.com>
> > > > > > > Cc: linux-pci@vger.kernel.org
> > > > > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > > > --
> > > > > > > v2:
> > > > > > > - Totally new approach: Adjust filp->f_mapping at open time. Note that
> > > > > > >   this now works on all architectures, not just those support
> > > > > > >   ARCH_GENERIC_PCI_MMAP_RESOURCE
> > > > > > > ---
> > > > > > >  drivers/pci/pci-sysfs.c | 4 ++++
> > > > > > >  drivers/pci/proc.c      | 1 +
> > > > > > >  2 files changed, 5 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > > > > > > index d15c881e2e7e..3f1c31bc0b7c 100644
> > > > > > > --- a/drivers/pci/pci-sysfs.c
> > > > > > > +++ b/drivers/pci/pci-sysfs.c
> > > > > > > @@ -929,6 +929,7 @@ void pci_create_legacy_files(struct pci_bus *b)
> > > > > > >         b->legacy_io->read = pci_read_legacy_io;
> > > > > > >         b->legacy_io->write = pci_write_legacy_io;
> > > > > > >         b->legacy_io->mmap = pci_mmap_legacy_io;
> > > > > > > +       b->legacy_io->mapping = iomem_get_mapping();
> > > > > > >         pci_adjust_legacy_attr(b, pci_mmap_io);
> > > > > > >         error = device_create_bin_file(&b->dev, b->legacy_io);
> > > > > > >         if (error)
> > > > > > > @@ -941,6 +942,7 @@ void pci_create_legacy_files(struct pci_bus *b)
> > > > > > >         b->legacy_mem->size = 1024*1024;
> > > > > > >         b->legacy_mem->attr.mode = 0600;
> > > > > > >         b->legacy_mem->mmap = pci_mmap_legacy_mem;
> > > > > > > +       b->legacy_io->mapping = iomem_get_mapping();
> > > > > >
> > > > > > Unlike the normal pci stuff below, the legacy files here go boom
> > > > > > because they're set up much earlier in the boot sequence. This only
> > > > > > affects HAVE_PCI_LEGACY architectures, which aren't that many. So what
> > > > > > should we do here now:
> > > > > > - drop the devmem revoke for these
> > > > > > - rework the init sequence somehow to set up these files a lot later
> > > > > > - redo the sysfs patch so that it doesn't take an address_space
> > > > > > pointer, but instead a callback to get at that (since at open time
> > > > > > everything is set up). Imo rather ugly
> > > > > > - ditch this part of the series (since there's not really any takers
> > > > > > for the latter parts it might just not make sense to push for this)
> > > > > > - something else?
> > > > > >
> > > > > > Bjorn, Greg, thoughts?
> > > > >
> > > > > What sysfs patch are you referring to here?
> > > >
> > > > Currently in linux-next:
> > > >
> > > > commit 74b30195395c406c787280a77ae55aed82dbbfc7 (HEAD ->
> > > > topic/iomem-mmap-vs-gup, drm/topic/iomem-mmap-vs-gup)
> > > > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > Date:   Fri Nov 27 17:41:25 2020 +0100
> > > >
> > > >    sysfs: Support zapping of binary attr mmaps
> > > >
> > > > Or the patch right before this one in this submission here:
> > > >
> > > > https://lore.kernel.org/dri-devel/20201127164131.2244124-12-daniel.vetter@ffwll.ch/
> > >
> > > Ah.  Hm, a callback in the sysfs file logic seems really hairy, so I
> > > would prefer that not happen.  If no one really needs this stuff, why
> > > not just drop it like you mention?
> >
> > Well it is needed, but just on architectures I don't care about much.
> > Most relevant is perhaps powerpc (that's where Stephen hit the issue).
> > I do wonder whether we could move the legacy pci files setup to where
> > the modern stuff is set up from pci_create_resource_files() or maybe
> > pci_create_sysfs_dev_files() even for HAVE_PCI_LEGACY. I think that
> > might work, but since it's legacy flow on some funny architectures
> > (alpha, itanium, that kind of stuff) I have no idea what kind of
> > monsters I'm going to anger :-)
>
> Back from a week of vacation, I looked at this again and I think
> shouldn't be hard to fix this with the sam trick
> pci_create_sysfs_dev_files() uses: As long as sysfs_initialized isn't
> set we skip, and then later on when the vfs is up&running we can
> initialize everything.
>
> To be able to apply the same thing to pci_create_legacy_files() I
> think all I need is to iterate overa all struct pci_bus in
> pci_sysfs_init() and we're good. Unfortunately I didn't find any
> for_each_pci_bus(), so how do I do that?

pci_find_next_bus() seems to be the answer I want. I'll see whether
that works and then send out new patches.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 32+ messages in thread

end of thread, other threads:[~2021-02-04 10:25 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-27 16:41 [PATCH v7 00/17] follow_pfn and other iomap races Daniel Vetter
2020-11-27 16:41 ` [PATCH v7 01/17] drm/exynos: Stop using frame_vector helpers Daniel Vetter
2020-11-27 16:41 ` [PATCH v7 02/17] drm/exynos: Use FOLL_LONGTERM for g2d cmdlists Daniel Vetter
2020-11-27 16:41 ` [PATCH v7 03/17] misc/habana: Stop using frame_vector helpers Daniel Vetter
2020-11-27 16:41 ` [PATCH v7 04/17] misc/habana: Use FOLL_LONGTERM for userptr Daniel Vetter
2020-11-27 16:41 ` [PATCH v7 05/17] mm/frame-vector: Use FOLL_LONGTERM Daniel Vetter
2020-11-27 16:41 ` [PATCH v7 06/17] media: videobuf2: Move frame_vector into media subsystem Daniel Vetter
2020-11-27 16:41 ` [PATCH v7 07/17] mm: Close race in generic_access_phys Daniel Vetter
2020-11-27 16:41 ` [PATCH v7 08/17] PCI: Obey iomem restrictions for procfs mmap Daniel Vetter
2020-11-27 16:41 ` [PATCH v7 09/17] /dev/mem: Only set filp->f_mapping Daniel Vetter
2020-11-27 16:41 ` [PATCH v7 10/17] resource: Move devmem revoke code to resource framework Daniel Vetter
2020-11-27 16:41 ` [PATCH v7 11/17] sysfs: Support zapping of binary attr mmaps Daniel Vetter
2020-11-27 16:41 ` [PATCH v7 12/17] PCI: Revoke mappings like devmem Daniel Vetter
2021-01-19  8:17   ` Daniel Vetter
2021-01-19 14:32     ` Greg Kroah-Hartman
2021-01-19 14:34       ` Daniel Vetter
2021-01-19 15:20         ` Greg Kroah-Hartman
2021-01-19 16:03           ` Daniel Vetter
2021-02-03 16:14             ` Daniel Vetter
2021-02-04 10:23               ` Daniel Vetter
2020-11-27 16:41 ` [PATCH v7 13/17] mm: Add unsafe_follow_pfn Daniel Vetter
2020-11-27 16:41 ` [PATCH v7 14/17] media/videobuf1|2: Mark follow_pfn usage as unsafe Daniel Vetter
2021-01-19 12:24   ` Hans Verkuil
2020-11-27 16:41 ` [PATCH v7 15/17] vfio/type1: Mark follow_pfn " Daniel Vetter
2020-11-27 16:41 ` [PATCH v7 16/17] kvm: pass kvm argument to follow_pfn callsites Daniel Vetter
2020-11-27 16:41 ` [PATCH v7 17/17] mm: add mmu_notifier argument to follow_pfn Daniel Vetter
2020-11-27 19:10   ` kernel test robot
2020-11-30 14:28     ` Daniel Vetter
2020-11-30 18:03       ` Nick Desaulniers
2021-01-12 13:24 ` [PATCH v7 00/17] follow_pfn and other iomap races Daniel Vetter
2021-01-12 13:28   ` Daniel Vetter
2021-01-12 20:57   ` Stephen Rothwell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).