Re: [PATCH RESEND] media: videobuf2: revert "get_userptr: buffers are always writable"
From: Tomasz Figa
Date: Mon Nov 28 2022 - 03:56:58 EST
On Mon, Nov 28, 2022 at 5:24 PM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote:
>
> Commit 707947247e95 ("media: videobuf2-vmalloc: get_userptr: buffers are
> always writable") caused problems in a corner case (passing read-only
> shmem memory as a userptr). So revert this patch.
>
> The original problem for which that commit was originally made is
> something that I could not reproduce after reverting it. So just go
> back to the way it was for many years, and if problems arise in
> the future, then another approach should be taken to resolve it.
>
> This patch is based on a patch from Hirokazu.
>
> Fixes: 707947247e95 ("media: videobuf2-vmalloc: get_userptr: buffers are always writable")
> Signed-off-by: Hirokazu Honda <hiroh@xxxxxxxxxxxx>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> ---
> CCed also to Andrew, linux-mm and linux-kernel. This patch is meant to be
> first in David Hildenbrand's 'remove FOLL_FORCE' series to ensure that it
> will be easy to backport this fix to older kernels without introducing new
> merge conflicts.
>
> Hans
> ---
> drivers/media/common/videobuf2/frame_vector.c | 10 +++++++---
> drivers/media/common/videobuf2/videobuf2-dma-contig.c | 3 ++-
> drivers/media/common/videobuf2/videobuf2-dma-sg.c | 4 +++-
> drivers/media/common/videobuf2/videobuf2-memops.c | 6 ++++--
> drivers/media/common/videobuf2/videobuf2-vmalloc.c | 4 +++-
> include/media/frame_vector.h | 2 +-
> include/media/videobuf2-memops.h | 3 ++-
> 7 files changed, 22 insertions(+), 10 deletions(-)
>
Thanks!
Acked-by: Tomasz Figa <tfiga@xxxxxxxxxxxx>
Best regards,
Tomasz
> diff --git a/drivers/media/common/videobuf2/frame_vector.c b/drivers/media/common/videobuf2/frame_vector.c
> index 542dde9d2609..aad72640f055 100644
> --- a/drivers/media/common/videobuf2/frame_vector.c
> +++ b/drivers/media/common/videobuf2/frame_vector.c
> @@ -14,6 +14,7 @@
> * get_vaddr_frames() - map virtual addresses to pfns
> * @start: starting user address
> * @nr_frames: number of pages / pfns from start to map
> + * @write: the mapped address has write permission
> * @vec: structure which receives pages / pfns of the addresses mapped.
> * It should have space for at least nr_frames entries.
> *
> @@ -32,7 +33,7 @@
> *
> * This function takes care of grabbing mmap_lock as necessary.
> */
> -int get_vaddr_frames(unsigned long start, unsigned int nr_frames,
> +int get_vaddr_frames(unsigned long start, unsigned int nr_frames, bool write,
> struct frame_vector *vec)
> {
> struct mm_struct *mm = current->mm;
> @@ -40,6 +41,7 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames,
> int ret_pin_user_pages_fast = 0;
> int ret = 0;
> int err;
> + unsigned int gup_flags = FOLL_FORCE | FOLL_LONGTERM;
>
> if (nr_frames == 0)
> return 0;
> @@ -49,8 +51,10 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames,
>
> start = untagged_addr(start);
>
> - ret = pin_user_pages_fast(start, nr_frames,
> - FOLL_FORCE | FOLL_WRITE | FOLL_LONGTERM,
> + if (write)
> + gup_flags |= FOLL_WRITE;
> +
> + ret = pin_user_pages_fast(start, nr_frames, gup_flags,
> (struct page **)(vec->ptrs));
> if (ret > 0) {
> vec->got_ref = true;
> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> index 678b359717c4..8e55468cb60d 100644
> --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> @@ -603,7 +603,8 @@ static void *vb2_dc_get_userptr(struct vb2_buffer *vb, struct device *dev,
> buf->vb = vb;
>
> offset = lower_32_bits(offset_in_page(vaddr));
> - vec = vb2_create_framevec(vaddr, size);
> + vec = vb2_create_framevec(vaddr, size, buf->dma_dir == DMA_FROM_DEVICE ||
> + buf->dma_dir == DMA_BIDIRECTIONAL);
> if (IS_ERR(vec)) {
> ret = PTR_ERR(vec);
> goto fail_buf;
> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> index fa69158a65b1..099693e42bc6 100644
> --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> @@ -241,7 +241,9 @@ static void *vb2_dma_sg_get_userptr(struct vb2_buffer *vb, struct device *dev,
> buf->size = size;
> buf->dma_sgt = &buf->sg_table;
> buf->vb = vb;
> - vec = vb2_create_framevec(vaddr, size);
> + vec = vb2_create_framevec(vaddr, size,
> + buf->dma_dir == DMA_FROM_DEVICE ||
> + buf->dma_dir == DMA_BIDIRECTIONAL);
> if (IS_ERR(vec))
> goto userptr_fail_pfnvec;
> buf->vec = vec;
> diff --git a/drivers/media/common/videobuf2/videobuf2-memops.c b/drivers/media/common/videobuf2/videobuf2-memops.c
> index 9dd6c27162f4..f9a4ec44422e 100644
> --- a/drivers/media/common/videobuf2/videobuf2-memops.c
> +++ b/drivers/media/common/videobuf2/videobuf2-memops.c
> @@ -26,6 +26,7 @@
> * vb2_create_framevec() - map virtual addresses to pfns
> * @start: Virtual user address where we start mapping
> * @length: Length of a range to map
> + * @write: Should we map for writing into the area
> *
> * This function allocates and fills in a vector with pfns corresponding to
> * virtual address range passed in arguments. If pfns have corresponding pages,
> @@ -34,7 +35,8 @@
> * failure. Returned vector needs to be freed via vb2_destroy_pfnvec().
> */
> struct frame_vector *vb2_create_framevec(unsigned long start,
> - unsigned long length)
> + unsigned long length,
> + bool write)
> {
> int ret;
> unsigned long first, last;
> @@ -47,7 +49,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, vec);
> + ret = get_vaddr_frames(start & PAGE_MASK, nr, write, vec);
> if (ret < 0)
> goto out_destroy;
> /* We accept only complete set of PFNs */
> diff --git a/drivers/media/common/videobuf2/videobuf2-vmalloc.c b/drivers/media/common/videobuf2/videobuf2-vmalloc.c
> index 948152f1596b..67d0b89e701b 100644
> --- a/drivers/media/common/videobuf2/videobuf2-vmalloc.c
> +++ b/drivers/media/common/videobuf2/videobuf2-vmalloc.c
> @@ -85,7 +85,9 @@ static void *vb2_vmalloc_get_userptr(struct vb2_buffer *vb, struct device *dev,
> buf->dma_dir = vb->vb2_queue->dma_dir;
> offset = vaddr & ~PAGE_MASK;
> buf->size = size;
> - vec = vb2_create_framevec(vaddr, size);
> + vec = vb2_create_framevec(vaddr, size,
> + buf->dma_dir == DMA_FROM_DEVICE ||
> + buf->dma_dir == DMA_BIDIRECTIONAL);
> if (IS_ERR(vec)) {
> ret = PTR_ERR(vec);
> goto fail_pfnvec_create;
> diff --git a/include/media/frame_vector.h b/include/media/frame_vector.h
> index bfed1710dc24..541c71a2c7be 100644
> --- a/include/media/frame_vector.h
> +++ b/include/media/frame_vector.h
> @@ -16,7 +16,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,
> - struct frame_vector *vec);
> + bool write, 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/include/media/videobuf2-memops.h b/include/media/videobuf2-memops.h
> index cd4a46331531..4b5b84f93538 100644
> --- a/include/media/videobuf2-memops.h
> +++ b/include/media/videobuf2-memops.h
> @@ -34,7 +34,8 @@ struct vb2_vmarea_handler {
> extern const struct vm_operations_struct vb2_common_vm_ops;
>
> struct frame_vector *vb2_create_framevec(unsigned long start,
> - unsigned long length);
> + unsigned long length,
> + bool write);
> void vb2_destroy_framevec(struct frame_vector *vec);
>
> #endif
> --
> 2.35.1
>