Re: [PATCH for v5.2] videobuf2-core.c: always reacquire USERPTR memory

[Hans Verkuil <hverkuil@xs4all.nl>]

On 6/7/19 2:14 PM, Marek Szyprowski wrote:
> Hi Hans,
> 
> On 2019-06-07 14:01, Hans Verkuil wrote:
>> On 6/7/19 1:16 PM, Laurent Pinchart wrote:
>>> Hi Hans,
>>>
>>> Thank you for the patch.
>>>
>>> On Fri, Jun 07, 2019 at 10:45:31AM +0200, Hans Verkuil wrote:
>>>> The __prepare_userptr() function made the incorrect assumption that if the
>>>> same user pointer was used as the last one for which memory was acquired, then
>>>> there was no need to re-acquire the memory. This assumption was never properly
>>>> tested, and after doing that it became clear that this was in fact wrong.
>>> Could you explain in the commit message why the assumption is not
>>> correct ?
>> You can free the memory, then allocate it again and you can get the same pointer,
>> even though it is not necessarily using the same physical pages for the memory
>> that the kernel is still using for it.
>>
>> Worse, you can free the memory, then allocate only half the memory you need and
>> get back the same pointer. vb2 wouldn't notice this. And it seems to work (since
>> the original mapping still remains), but this can corrupt userspace memory
>> causing the application to crash. It's not quite clear to me how the memory can
>> get corrupted. I don't know enough of those low-level mm internals to understand
>> the sequence of events.
>>
>> I have test code for v4l2-compliance available if someone wants to test this.
> 
> I'm interested, I would really like to know what happens in the mm 
> subsystem in such case.

Here it is:

diff --git a/utils/v4l2-compliance/v4l2-test-buffers.cpp b/utils/v4l2-compliance/v4l2-test-buffers.cpp
index be606e48..9abf41da 100644
--- a/utils/v4l2-compliance/v4l2-test-buffers.cpp
+++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp
@@ -797,7 +797,7 @@ int testReadWrite(struct node *node)
 	return 0;
 }

-static int captureBufs(struct node *node, const cv4l_queue &q,
+static int captureBufs(struct node *node, cv4l_queue &q,
 		const cv4l_queue &m2m_q, unsigned frame_count, int pollmode,
 		unsigned &capture_count)
 {
@@ -962,6 +962,21 @@ static int captureBufs(struct node *node, const cv4l_queue &q,
 				buf.s_flags(V4L2_BUF_FLAG_REQUEST_FD);
 				buf.s_request_fd(buf_req_fds[req_idx]);
 			}
+			if (v4l_type_is_capture(buf.g_type()) && q.g_memory() == V4L2_MEMORY_USERPTR) {
+				printf("\nidx: %d", buf.g_index());
+				for (unsigned p = 0; p < q.g_num_planes(); p++) {
+					printf(" old buf[%d]: %p ", p, buf.g_userptr(p));
+					fflush(stdout);
+					free(buf.g_userptr(p));
+					void *m = calloc(1, q.g_length(p)/2);
+
+					fail_on_test(m == NULL);
+					q.s_userptr(buf.g_index(), p, m);
+					printf("new buf[%d]: %p", p, m);
+					buf.s_userptr(m, p);
+				}
+				printf("\n");
+			}
 			fail_on_test(buf.qbuf(node, q));
 			fail_on_test(buf.g_flags() & V4L2_BUF_FLAG_DONE);
 			if (buf.g_flags() & V4L2_BUF_FLAG_REQUEST_FD) {



Load the vivid driver and just run 'v4l2-compliance -s10' and you'll see:

...
Streaming ioctls:
        test read/write: OK
        test blocking wait: OK
        test MMAP (no poll): OK
        test MMAP (select): OK
        test MMAP (epoll): OK
        Video Capture: Frame #000
idx: 0 old buf[0]: 0x7f71c6e7c010 new buf[0]: 0x7f71c6eb4010
        Video Capture: Frame #001
idx: 1 old buf[0]: 0x7f71c6e0b010 new buf[0]: 0x7f71c6e7b010
        Video Capture: Frame #002
idx: 0 old buf[0]: 0x7f71c6eb4010 free(): invalid pointer
Aborted

Regards,

	Hans