Hi all
Round 3 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/[email protected]/
v2: https://lore.kernel.org/dri-devel/[email protected]
v3: https://lore.kernel.org/dri-devel/[email protected]/
And the discussion that sparked this journey:
https://lore.kernel.org/dri-devel/[email protected]/
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.
I feel like this is ready for some wider soaking. Since the remaining bits
are all kinda connnected probably simplest if it all goes through -mm.
Cheers, Daniel
Daniel Vetter (15):
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
mm: Add unsafe_follow_pfn
media/videbuf1|2: Mark follow_pfn usage as unsafe
vfio/type1: Mark follow_pfn as unsafe
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
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 | 54 ++++------
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 | 50 ++++-----
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/mm.h | 47 +-------
include/linux/sysfs.h | 2 +
include/media/frame_vector.h | 47 ++++++++
include/media/videobuf2-core.h | 1 +
kernel/resource.c | 101 +++++++++++++++++-
mm/Kconfig | 3 -
mm/Makefile | 1 -
mm/memory.c | 78 +++++++++++++-
mm/nommu.c | 17 +++
security/Kconfig | 13 +++
26 files changed, 347 insertions(+), 245 deletions(-)
rename {mm => drivers/media/common/videobuf2}/frame_vector.c (85%)
create mode 100644 include/media/frame_vector.h
--
2.28.0
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.
Reviewed-by: John Hubbard <[email protected]>
Signed-off-by: Daniel Vetter <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: Inki Dae <[email protected]>
Cc: Joonyoung Shim <[email protected]>
Cc: Seung-Woo Kim <[email protected]>
Cc: Kyungmin Park <[email protected]>
Cc: Kukjin Kim <[email protected]>
Cc: Krzysztof Kozlowski <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: John Hubbard <[email protected]>
Cc: Jérôme Glisse <[email protected]>
Cc: Jan Kara <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Daniel Vetter <[email protected]>
--
v2: Use unpin_user_pages_dirty_lock (John)
---
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 6417f374b923..43257ef3c09d 100644
--- a/drivers/gpu/drm/exynos/Kconfig
+++ b/drivers/gpu/drm/exynos/Kconfig
@@ -88,7 +88,6 @@ comment "Sub-drivers"
config DRM_EXYNOS_G2D
bool "G2D"
depends on VIDEO_SAMSUNG_S5P_G2D=n || COMPILE_TEST
- select FRAME_VECTOR
help
Choose this option if you want to use Exynos G2D for DRM.
diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
index 967a5cdc120e..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.28.0
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").
Signed-off-by: Daniel Vetter <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: John Hubbard <[email protected]>
Cc: Jérôme Glisse <[email protected]>
Cc: Jan Kara <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: Pawel Osciak <[email protected]>
Cc: Marek Szyprowski <[email protected]>
Cc: Kyungmin Park <[email protected]>
Cc: Tomasz Figa <[email protected]>
Cc: Laurent Dufour <[email protected]>
Cc: Vlastimil Babka <[email protected]>
Cc: Daniel Jordan <[email protected]>
Cc: Michel Lespinasse <[email protected]>
Signed-off-by: Daniel Vetter <[email protected]>
--
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 6590987c14bd..e630494da65c 100644
--- a/drivers/media/common/videobuf2/frame_vector.c
+++ b/drivers/media/common/videobuf2/frame_vector.c
@@ -69,7 +69,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.28.0
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 <[email protected]>
Acked-by: Mauro Carvalho Chehab <[email protected]>
Signed-off-by: Daniel Vetter <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: Pawel Osciak <[email protected]>
Cc: Marek Szyprowski <[email protected]>
Cc: Kyungmin Park <[email protected]>
Cc: Tomasz Figa <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: John Hubbard <[email protected]>
Cc: Jérôme Glisse <[email protected]>
Cc: Jan Kara <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: Daniel Vetter <[email protected]>
Signed-off-by: Daniel Vetter <[email protected]>
--
v3:
- Create a new frame_vector.h header for this (Mauro)
---
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 d44779e56313..6590987c14bd 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 16b799a0522c..acd60fbf1a5a 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1743,48 +1743,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,
- unsigned int gup_flags, 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..1ed0cd64510d
--- /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,
+ unsigned int gup_flags, 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 6c974888f86f..da6c943fe9f1 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -815,9 +815,6 @@ config DEVICE_PRIVATE
memory; i.e., memory that is only accessible from the device (or
group of devices). You likely also want to select HMM_MIRROR.
-config FRAME_VECTOR
- bool
-
config ARCH_USES_HIGH_VMA_FLAGS
bool
config ARCH_HAS_PKEYS
diff --git a/mm/Makefile b/mm/Makefile
index d5649f1c12c0..a025fd6c6afd 100644
--- a/mm/Makefile
+++ b/mm/Makefile
@@ -111,7 +111,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.28.0
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 <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: John Hubbard <[email protected]>
Cc: Jérôme Glisse <[email protected]>
Cc: Jan Kara <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: Alex Williamson <[email protected]>
Cc: Cornelia Huck <[email protected]>
Cc: [email protected]
Signed-off-by: Daniel Vetter <[email protected]>
---
drivers/vfio/vfio_iommu_type1.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 5fbf0c1f7433..a4d53f3d0a35 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -421,7 +421,7 @@ static int follow_fault_pfn(struct vm_area_struct *vma, struct mm_struct *mm,
{
int ret;
- ret = follow_pfn(vma, vaddr, pfn);
+ ret = unsafe_follow_pfn(vma, vaddr, pfn);
if (ret) {
bool unlocked = false;
@@ -435,7 +435,7 @@ static int follow_fault_pfn(struct vm_area_struct *vma, struct mm_struct *mm,
if (ret)
return ret;
- ret = follow_pfn(vma, vaddr, pfn);
+ ret = unsafe_follow_pfn(vma, vaddr, pfn);
}
return ret;
--
2.28.0
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 <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: Inki Dae <[email protected]>
Cc: Joonyoung Shim <[email protected]>
Cc: Seung-Woo Kim <[email protected]>
Cc: Kyungmin Park <[email protected]>
Cc: Kukjin Kim <[email protected]>
Cc: Krzysztof Kozlowski <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: John Hubbard <[email protected]>
Cc: Jérôme Glisse <[email protected]>
Cc: Jan Kara <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Daniel Vetter <[email protected]>
---
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.28.0
Hi Daniel,
On Mon, Oct 26, 2020 at 11:58:12AM +0100, 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").
Note that this is true only for the videobuf2 change. The videobuf1 code
was like this all the time and does not support normal memory in the
dma_contig variant (because normal memory is rarely physically contiguous).
If my understanding is correct that the CONFIG_STRICT_FOLLOW_PFN is not
enabled by default, we stay backwards compatible, with only whoever
decides to turn it on risking a breakage.
I agree that this is a good first step towards deprecating this legacy
code, so:
Acked-by: Tomasz Figa <[email protected]>
Of course the last word goes to Mauro. :)
Best regards,
Tomasz
>
> Signed-off-by: Daniel Vetter <[email protected]>
> Cc: Jason Gunthorpe <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: John Hubbard <[email protected]>
> Cc: J?r?me Glisse <[email protected]>
> Cc: Jan Kara <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: Pawel Osciak <[email protected]>
> Cc: Marek Szyprowski <[email protected]>
> Cc: Kyungmin Park <[email protected]>
> Cc: Tomasz Figa <[email protected]>
> Cc: Laurent Dufour <[email protected]>
> Cc: Vlastimil Babka <[email protected]>
> Cc: Daniel Jordan <[email protected]>
> Cc: Michel Lespinasse <[email protected]>
> Signed-off-by: Daniel Vetter <[email protected]>
> --
> 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 6590987c14bd..e630494da65c 100644
> --- a/drivers/media/common/videobuf2/frame_vector.c
> +++ b/drivers/media/common/videobuf2/frame_vector.c
> @@ -69,7 +69,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.28.0
>
On Mon, Oct 26, 2020 at 11:58:09AM +0100, Daniel Vetter wrote:
> It's the only user. This also garbage collects the CONFIG_FRAME_VECTOR
> symbol from all over the tree (well just one place, somehow omap media
> driver still had this in its Kconfig, despite not using it).
>
> Reviewed-by: John Hubbard <[email protected]>
> Acked-by: Mauro Carvalho Chehab <[email protected]>
> Signed-off-by: Daniel Vetter <[email protected]>
> Cc: Jason Gunthorpe <[email protected]>
> Cc: Pawel Osciak <[email protected]>
> Cc: Marek Szyprowski <[email protected]>
> Cc: Kyungmin Park <[email protected]>
> Cc: Tomasz Figa <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: John Hubbard <[email protected]>
> Cc: J?r?me Glisse <[email protected]>
> Cc: Jan Kara <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: Daniel Vetter <[email protected]>
> Signed-off-by: Daniel Vetter <[email protected]>
> --
> v3:
> - Create a new frame_vector.h header for this (Mauro)
> ---
> 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
>
Acked-by: Tomasz Figa <[email protected]>
Best regards,
Tomasz
> 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 d44779e56313..6590987c14bd 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 16b799a0522c..acd60fbf1a5a 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1743,48 +1743,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,
> - unsigned int gup_flags, 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..1ed0cd64510d
> --- /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,
> + unsigned int gup_flags, 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 6c974888f86f..da6c943fe9f1 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -815,9 +815,6 @@ config DEVICE_PRIVATE
> memory; i.e., memory that is only accessible from the device (or
> group of devices). You likely also want to select HMM_MIRROR.
>
> -config FRAME_VECTOR
> - bool
> -
> config ARCH_USES_HIGH_VMA_FLAGS
> bool
> config ARCH_HAS_PKEYS
> diff --git a/mm/Makefile b/mm/Makefile
> index d5649f1c12c0..a025fd6c6afd 100644
> --- a/mm/Makefile
> +++ b/mm/Makefile
> @@ -111,7 +111,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.28.0
>
Maybe I'm missing something, but shouldn't follow_pfn be unexported
at the end of the series?
On Thu, Oct 29, 2020 at 9:57 AM Christoph Hellwig <[email protected]> wrote:
>
> Maybe I'm missing something, but shouldn't follow_pfn be unexported
> at the end of the series?
kvm is a legit user and modular afaict. But since you can't use this
without an mmu_notifier anyway (or digging around in pagetable
locking), maybe it should be EXPORT_SYMBOL_GPL now at least?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Thu, Oct 29, 2020 at 10:25:16AM +0100, Daniel Vetter wrote:
> On Thu, Oct 29, 2020 at 9:57 AM Christoph Hellwig <[email protected]> wrote:
> >
> > Maybe I'm missing something, but shouldn't follow_pfn be unexported
> > at the end of the series?
>
> kvm is a legit user and modular afaict. But since you can't use this
> without an mmu_notifier anyway (or digging around in pagetable
> locking), maybe it should be EXPORT_SYMBOL_GPL now at least?
I think it should then take the notifier as an argument even if it isn't
diretly used as a safety check, and get a new name describing it.
EXPORT_SYMBOL_GPL is probably ok for now, but I'm drafting a new
EXPORT_SYMBOL_FOR_MODULE() which will export symbols that can only be
used by one specific module, with kvm being a prime user due to all
the odd exports it requires that aren't really the kernel interface by
any normal means.
On Thu, Oct 29, 2020 at 10:28 AM Christoph Hellwig <[email protected]> wrote:
>
> On Thu, Oct 29, 2020 at 10:25:16AM +0100, Daniel Vetter wrote:
> > On Thu, Oct 29, 2020 at 9:57 AM Christoph Hellwig <[email protected]> wrote:
> > >
> > > Maybe I'm missing something, but shouldn't follow_pfn be unexported
> > > at the end of the series?
> >
> > kvm is a legit user and modular afaict. But since you can't use this
> > without an mmu_notifier anyway (or digging around in pagetable
> > locking), maybe it should be EXPORT_SYMBOL_GPL now at least?
>
> I think it should then take the notifier as an argument even if it isn't
> diretly used as a safety check, and get a new name describing it.
Hm so Jason and me discussed this, but e.g. the s390 is safe with with
just the pagetable locks. So we'd need two versions.
The more practical problem is that I haven't found a reasonable way to
check that a passed in mmu_notifier is registered against the mm we're
working on, and without that check it feels a bit silly. But if you
see how to do that I think we can do an EXPORT_SYMBOL_GPL follow_pfn
which takes the notifier, and an __follow_pfn for s390 and similar
internal code which isn't exported.
> EXPORT_SYMBOL_GPL is probably ok for now, but I'm drafting a new
> EXPORT_SYMBOL_FOR_MODULE() which will export symbols that can only be
> used by one specific module, with kvm being a prime user due to all
> the odd exports it requires that aren't really the kernel interface by
> any normal means.
Hm yeah that's another one. There's also some virt stuff that's
currently on unsafe_follow_pfn and needs to be switched over, and I
think that would also need an mmu notifier of some sorts to close the
gaps.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Thu, Oct 29, 2020 at 10:38:16AM +0100, Daniel Vetter wrote:
> Hm so Jason and me discussed this, but e.g. the s390 is safe with with
> just the pagetable locks. So we'd need two versions.
>
> The more practical problem is that I haven't found a reasonable way to
> check that a passed in mmu_notifier is registered against the mm we're
> working on, and without that check it feels a bit silly. But if you
> see how to do that I think we can do an EXPORT_SYMBOL_GPL follow_pfn
> which takes the notifier, and an __follow_pfn for s390 and similar
> internal code which isn't exported.
True, this is a bit of a mess. So maybe just rename it to __follow_pfn,
proper documentation of the requirements and a switch to
EXPORT_SYMBOL_GPL.