Hi Hans, On Tuesday 04 December 2012 16:48:40 Hans Verkuil wrote: > (repost after accidentally using HTML formatting) > > This needs a good review. The change is minor, but as I am not a mm expert, > I'd like to get some more feedback on this. The dma-sg change has been > successfully tested on our hardware, but I don't have any hardware to test > the vmalloc change. > > Note that the 'write' attribute is still stored internally and used to tell > whether set_page_dirty_lock() should be called during put_userptr. > > It is my understanding that that still makes sense, so I didn't change that. > > Regards, > > Hans > > --- start patch --- > > When calling get_user_pages for output buffers, the 'write' argument is set > to 0 (since the DMA isn't writing to memory), This can cause unexpected > results: > > If you calloc() buffer memory and do not fill that memory afterwards, then > the kernel assigns most of that memory to one single physical 'zero' page. > > If you queue that buffer to the V4L2 driver, then it will call > get_user_pages and store the results. Next you dequeue it, fill the buffer > and queue it again. Now the V4L2 core code sees the same userptr and length > and expects it to be the same buffer that it got before and it will reuse > the results of the previous get_user_pages call. But that still points to > zero pages, whereas userspace filled it up and so changed the buffer to use > different pages. In other words, the pages the V4L2 core knows about are no > longer correct. > > The solution is to always set 'write' to 1 as this will force the kernel to > fault in proper pages. I'm wondering if we should really force faulting pages. The buffer given to the driver might be real read-only memory, in which case faulting the pages would probably hurt (agreed, that's pretty unlikely, but it's still a valid use case according to the API). Moreover, this wouldn't fix all similar userptr-related issues. An application could remap a completely different memory area to the same userspace address between two QBUF calls, and videobuf2 would not handle that properly. That's also a valid use case according to the API, and it would be pretty difficult to handle it correctly in an efficient way on the kernel side. I think we could modify the spec to forbid mapping changes between QBUF calls, and in that case the zero-page mapping situation you described could be forbidden as well. If we clearly document it in the spec we could push the responsibility of faulting the pages to userspace. > We do this for videobuf2-dma-sg.c and videobuf2-vmalloc.c, but not for > videobuf2-dma-contig.c since the userptr there is already supposed to > point to contiguous memory and shouldn't use the zero page at all. > > Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx> > --- > drivers/media/v4l2-core/videobuf2-dma-sg.c | 3 ++- > drivers/media/v4l2-core/videobuf2-vmalloc.c | 4 +++- > 2 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c > b/drivers/media/v4l2-core/videobuf2-dma-sg.c index 25c3b36..c29f159 100644 > --- a/drivers/media/v4l2-core/videobuf2-dma-sg.c > +++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c > @@ -143,7 +143,8 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx, > unsigned long vaddr, num_pages_from_user = get_user_pages(current, > current->mm, > vaddr & PAGE_MASK, > buf->sg_desc.num_pages, > - write, > + 1, /* always set write to force > + faulting all pages */ > 1, /* force */ > buf->pages, > NULL); > diff --git a/drivers/media/v4l2-core/videobuf2-vmalloc.c > b/drivers/media/v4l2-core/videobuf2-vmalloc.c index a47fd4f..c8d8519 100644 > --- a/drivers/media/v4l2-core/videobuf2-vmalloc.c > +++ b/drivers/media/v4l2-core/videobuf2-vmalloc.c > @@ -107,7 +107,9 @@ static void *vb2_vmalloc_get_userptr(void *alloc_ctx, > unsigned long vaddr, /* current->mm->mmap_sem is taken by videobuf2 core */ > n_pages = get_user_pages(current, current->mm, > vaddr & PAGE_MASK, buf->n_pages, > - write, 1, /* force */ > + 1, /* always set write to force > + faulting all pages */ > + 1, /* force */ > buf->pages, NULL); > if (n_pages != buf->n_pages) > goto fail_get_user_pages; -- 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
