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
