Hi Daniel,
On Fri, Oct 09, 2020 at 07:52:05PM +0200, Daniel Vetter wrote:
On Fri, Oct 9, 2020 at 2:48 PM Jason Gunthorpe jgg@ziepe.ca wrote:
On Fri, Oct 09, 2020 at 02:37:23PM +0200, Mauro Carvalho Chehab wrote:
I'm not a mm/ expert, but, from what I understood from Daniel's patch description is that this is unsafe *only if* __GFP_MOVABLE is used.
No, it is unconditionally unsafe. The CMA movable mappings are specific VMAs that will have bad issues here, but there are other types too.
The only way to do something at a VMA level is to have a list of OK VMAs, eg because they were creatd via a special mmap helper from the media subsystem.
Well, no drivers inside the media subsystem uses such flag, although they may rely on some infrastructure that could be using it behind the bars.
It doesn't matter, nothing prevents the user from calling media APIs on mmaps it gets from other subsystems.
I think a good first step would be to disable userptr of non struct page backed storage going forward for any new hw support. Even on existing drivers. dma-buf sharing has been around for long enough now that this shouldn't be a problem. Unfortunately right now this doesn't seem to exist, so the entire problem keeps getting perpetuated.
On the V4L2 side, I think we should disable USERPTR for any new driver, period. That's what I've been recommended when reviewing patches for several years already. It's a deprecated API, it should be phased out, which starts by not allowing any new use case.
If this is the case, the proper fix seems to have a GFP_NOT_MOVABLE flag that it would be denying the core mm code to set __GFP_MOVABLE.
We can't tell from the VMA these kinds of details..
It has to go the other direction, evey mmap that might be used as a userptr here has to be found and the VMA specially created to allow its use. At least that is a kernel only change, but will need people with the HW to do this work.
I think the only reasonable way to keep this working is:
- add a struct dma_buf *vma_tryget_dma_buf(struct vm_area_struct *vma);
- add dma-buf export support to fbdev and v4l
- roll this out everywhere we still need it.
Realistically this just isn't going to happen. And anything else just reimplements half of dma-buf, which is kinda pointless (you need minimally refcounting and some way to get at a promise of a permanent sg list for dma. Plus probably the vmap for kernel cpu access.
Please let address the issue on this way, instead of broken an userspace API that it is there since 1991.
It has happened before :( It took 4 years for RDMA to undo the uAPI breakage caused by a security fix for something that was a 15 years old bug.
Yeah we have a bunch of these on the drm side too. Some of them are really just "you have to upgrade userspace", and there's no real fix for the security nightmare without that.