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

From: Hans Verkuil <hverkuil@xs4all.nl>
To: Tomasz Figa <tfiga@chromium.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Linux Media Mailing List <linux-media@vger.kernel.org>,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	Sakari Ailus <sakari.ailus@linux.intel.com>
Subject: Re: [PATCH for v5.2] videobuf2-core.c: always reacquire USERPTR memory
Date: Fri, 7 Jun 2019 14:24:41 +0200	[thread overview]
Message-ID: <c4bfba37-dc6c-43cd-c91c-1472d2f500c1@xs4all.nl> (raw)
In-Reply-To: <CAAFQd5CTgm0bimJOzdFUdM35Bro4xsLwa698E7pR_hQ=ebaM3g@mail.gmail.com>

On 6/7/19 2:20 PM, Tomasz Figa wrote:
> On Fri, Jun 7, 2019 at 9:01 PM Hans Verkuil <hverkuil@xs4all.nl> 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.
> 
> Chrome specifically didn't keep the mapping between user pointers and
> indexes, so it the cache just missed every time. What we noticed was
> the put_userptr on the previous userptr at the index being unmapped
> apparently caused that memory (often already returned back to the
> application) to be corrupted... But we didn't get to the bottom of it
> either, as we didn't have any MM expert look at the issue.

I think this patch needs a bit more work. The put_userptr should happen
before the buffer is dequeued to userspace, not when queuing a new buffer.

I'll make a v2.

Regards,

	Hans

> 
> The free and realloc scenario just came to my mind when trying to
> recall our original problem earlier today.
> 
> Best regards,
> Tomasz
> 