Re: [RFC PATCH] vb2: force output buffers to fault into memory

[Marek Szyprowski <m.szyprowski@samsung.com>]

Hello,

On 12/6/2012 2:23 AM, Laurent Pinchart wrote:
> 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.

I've spent some time thinking on this issue and I came to the conclusion 
that it
might be a good idea extend the v4l2 spec with such case. Changing the 
write
parameter for get_user_pages() is only a workaround, which doesn't even 
work for
all possible use cases, so I agree with Laurent that user ptr usage 
should be
much better documented. I only wonder if it a good idea to require 
faulting in
pages from the user space application. Can we assume that memset(ptr, 0, 
buf_size)
will do the job in all cases? I expect yes, but I would like to be 
really sure,
this will be probably put into the documentation for the reference code.

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center