On Wed, Oct 7, 2020 at 3:06 PM Jason Gunthorpe jgg@ziepe.ca wrote:
On Wed, Oct 07, 2020 at 02:58:33PM +0200, Daniel Vetter wrote:
On Wed, Oct 7, 2020 at 2:48 PM Tomasz Figa tfiga@chromium.org wrote:
On Wed, Oct 7, 2020 at 2:44 PM Jason Gunthorpe jgg@ziepe.ca wrote:
On Wed, Oct 07, 2020 at 02:33:56PM +0200, Marek Szyprowski wrote:
Well, it was in vb2_get_vma() function, but now I see that it has been lost in fb639eb39154 and 6690c8c78c74 some time ago...
There is no guarentee that holding a get on the file says anthing about the VMA. This needed to check that the file was some special kind of file that promised the VMA layout and file lifetime are connected.
Also, cloning a VMA outside the mm world is just really bad. That would screw up many assumptions the drivers make.
If it is all obsolete I say we hide it behind a default n config symbol and taint the kernel if anything uses it.
Add a big comment above the follow_pfn to warn others away from this code.
Sadly it's just verbally declared as deprecated and not formally noted anyway. There are a lot of userspace applications relying on user pointer support.
userptr can stay, it's the userptr abuse for zerocpy buffer sharing which doesn't work anymore. At least without major surgery (you'd need an mmu notifier to zap mappings and recreate them, and that pretty much breaks the v4l model of preallocating all buffers to make sure we never underflow the buffer queue). And static mappings are not coming back I think, we'll go ever more into the direction of dynamic mappings and moving stuff around as needed.
Right, and to be clear, the last time I saw a security flaw of this magnitude from a subsystem badly mis-designing itself, Linus's knee-jerk reaction was to propose to remove the whole subsystem.
Please don't take status-quo as acceptable, V4L community has to work to resolve this, uABI breakage or not. The follow_pfn related code must be compiled out of normal distro kernel builds.
I think the userptr zero-copy hack should be able to go away indeed, given that we now have CMA that allows having carveouts backed by struct pages and having the memory represented as DMA-buf normally.
How about the regular userptr use case, though?
The existing code resolves the user pointer into pages by following the get_vaddr_frames() -> frame_vector_to_pages() -> sg_alloc_table_from_pages() / vm_map_ram() approach. get_vaddr_frames() seems to use pin_user_pages() behind the scenes if the vma is not an IO or a PFNMAP, falling back to follow_pfn() otherwise.
Is your intention to drop get_vaddr_frames() or we could still keep using it and if vec->is_pfns is true: a) if CONFIG_VIDEO_LEGACY_PFN_USERPTR is set, taint the kernel b) otherwise just undo and fail?
Best regards, Tomasz