Hello,
(CC'ing Tomi Valkeinen)
On Wednesday 10 June 2015 06:20:45 Mauro Carvalho Chehab wrote:
> From: Jan Kara <jack@xxxxxxx>
>
> Convert omap_vout_uservirt_to_phys() to use get_vaddr_pfns() instead of
> hand made mapping of virtual address to physical address. Also the
> function leaked page reference from get_user_pages() so fix that by
> properly release the reference when omap_vout_buffer_release() is
> called.
>
> Signed-off-by: Jan Kara <jack@xxxxxxx>
> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> [hans.verkuil@xxxxxxxxx: remove unused struct omap_vout_device *vout
> variable]
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxx>
>
> diff --git a/drivers/media/platform/omap/omap_vout.c
> b/drivers/media/platform/omap/omap_vout.c index f09c5f17a42f..7feb6394f111
> 100644
> --- a/drivers/media/platform/omap/omap_vout.c
> +++ b/drivers/media/platform/omap/omap_vout.c
> @@ -195,46 +195,34 @@ static int omap_vout_try_format(struct v4l2_pix_format
> *pix) }
>
> /*
> - * omap_vout_uservirt_to_phys: This inline function is used to convert user
> - * space virtual address to physical address.
> + * omap_vout_get_userptr: Convert user space virtual address to physical
> + * address.
> */
> -static unsigned long omap_vout_uservirt_to_phys(unsigned long virtp)
> +static int omap_vout_get_userptr(struct videobuf_buffer *vb, u32 virtp,
> + u32 *physp)
> {
> - unsigned long physp = 0;
> - struct vm_area_struct *vma;
> - struct mm_struct *mm = current->mm;
> + struct frame_vector *vec;
> + int ret;
>
> /* For kernel direct-mapped memory, take the easy way */
> - if (virtp >= PAGE_OFFSET)
> - return virt_to_phys((void *) virtp);
> -
> - down_read(¤t->mm->mmap_sem);
> - vma = find_vma(mm, virtp);
> - if (vma && (vma->vm_flags & VM_IO) && vma->vm_pgoff) {
> - /* this will catch, kernel-allocated, mmaped-to-usermode
> - addresses */
> - physp = (vma->vm_pgoff << PAGE_SHIFT) + (virtp - vma->vm_start);
> - up_read(¤t->mm->mmap_sem);
> - } else {
> - /* otherwise, use get_user_pages() for general userland pages */
> - int res, nr_pages = 1;
> - struct page *pages;
> + if (virtp >= PAGE_OFFSET) {
> + *physp = virt_to_phys((void *)virtp);
Lovely. virtp comes from userspace and as far as I know it arrives here
completely unchecked. The problem isn't introduced by this patch, but
omap_vout buffer management seems completely broken to me, and nobody seems to
care about the driver. Given that omapdrm should now provide the video output
capabilities that are missing from omapfb and resulted in the development of
omap_vout, shouldn't we drop the omap_vout driver ?
Tomi, any opinion on this ? Do you see any omap_vout capability missing from
omapdrm ?
> + return 0;
> + }
>
> - res = get_user_pages(current, current->mm, virtp, nr_pages, 1,
> - 0, &pages, NULL);
> - up_read(¤t->mm->mmap_sem);
> + vec = frame_vector_create(1);
> + if (!vec)
> + return -ENOMEM;
>
> - if (res == nr_pages) {
> - physp = __pa(page_address(&pages[0]) +
> - (virtp & ~PAGE_MASK));
> - } else {
> - printk(KERN_WARNING VOUT_NAME
> - "get_user_pages failed\n");
> - return 0;
> - }
> + ret = get_vaddr_frames(virtp, 1, true, false, vec);
> + if (ret != 1) {
> + frame_vector_destroy(vec);
> + return -EINVAL;
> }
> + *physp = __pfn_to_phys(frame_vector_pfns(vec)[0]);
> + vb->priv = vec;
>
> - return physp;
> + return 0;
> }
>
> /*
> @@ -784,11 +772,15 @@ static int omap_vout_buffer_prepare(struct
> videobuf_queue *q, * address of the buffer
> */
> if (V4L2_MEMORY_USERPTR == vb->memory) {
> + int ret;
> +
> if (0 == vb->baddr)
> return -EINVAL;
> /* Physical address */
> - vout->queued_buf_addr[vb->i] = (u8 *)
> - omap_vout_uservirt_to_phys(vb->baddr);
> + ret = omap_vout_get_userptr(vb, vb->baddr,
> + (u32 *)&vout->queued_buf_addr[vb->i]);
> + if (ret < 0)
> + return ret;
> } else {
> unsigned long addr, dma_addr;
> unsigned long size;
> @@ -834,12 +826,13 @@ static void omap_vout_buffer_queue(struct
> videobuf_queue *q, static void omap_vout_buffer_release(struct
> videobuf_queue *q,
> struct videobuf_buffer *vb)
> {
> - struct omap_vout_device *vout = q->priv_data;
> -
> vb->state = VIDEOBUF_NEEDS_INIT;
> + if (vb->memory == V4L2_MEMORY_USERPTR && vb->priv) {
> + struct frame_vector *vec = vb->priv;
>
> - if (V4L2_MEMORY_MMAP != vout->memory)
> - return;
> + put_vaddr_frames(vec);
> + frame_vector_destroy(vec);
> + }
> }
>
> /*
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
