On 6/7/19 2:23 PM, Hans Verkuil wrote:
> 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
To clarify: two full size buffers are allocated and queued (that happens in setupUserPtr()),
then streaming starts and captureBufs is called which basically just calls dqbuf
and qbuf.
Tomasz pointed out that all the pointers in this log are actually different. That's
correct, but here is a log where the old and new buf ptr are the same:
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]: 0x7f1094e16010 new buf[0]: 0x7f1094e4e010
Video Capture: Frame #001
idx: 1 old buf[0]: 0x7f1094da5010 new buf[0]: 0x7f1094e15010
Video Capture: Frame #002
idx: 0 old buf[0]: 0x7f1094e4e010 new buf[0]: 0x7f1094e4e010
Video Capture: Frame #003
idx: 1 old buf[0]: 0x7f1094e15010 free(): invalid pointer
Aborted
It's weird that the first log fails that way: if the pointers are different,
then vb2 will call get_userptr and it should discover that the buffer isn't
large enough, causing qbuf to fail. That doesn't seem to happen.
Regards,
Hans
