On 6/7/19 2:20 PM, Tomasz Figa wrote: > On Fri, Jun 7, 2019 at 9:01 PM Hans Verkuil <hverkuil@xxxxxxxxx> 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 >
