[RFC] deadlock in "drm/exynos: fix wrong pointer access at vm close"

[Al Viro]

On Tue, Sep 24, 2013 at 01:41:00PM +0900, Inki Dae wrote:

> I can't see to hold ->mmap_sem when it calls find_vma() anywhere else.

Er...  What, in your opinion, would protect the result of find_vma(), if
not that?  E.g. if another thread does munmap() on that area...  vma isn't
refcounted; there are only two things that can prevent its freeing -
mmap_sem being held and the lack of anybody else seeing the address of
mm_struct it belongs to (basically, when we are killing mm_struct off
or when we are populating a fresh mm_struct; in the latter case the
parent is locked, but the child doesn't need to).

Look at page fault handlers - they grab mmap_sem (shared) before looking for
vma.  And anything modifying the list of vmas (vm_mmap(), etc.) grabs it
exclusive...

> > to caller) and clones it manually, regardless of whether that vma allows
> > to clone itself or not.  Quite a few drivers rely on that not happening...
> > 
> 
> I think that has no any problem because exynos_gem_get_vma() takes a
> reference to vma, and also v4l2 side is using same way. I and v4l2 guys
> might be missing something what you are concerning. So Could you give us
> more comments?

vb2_get_vma()/vb2_put_userptr() is also buggy.  At the very least, it
should refuse to handle ones with VM_DONTCOPY in flags.  Again, there
are drivers that seriously rely on VM_DONTCOPY being honoured.

BTW, what do you expect exynos_gem_get_pages_from_userptr() to do if
the area has been unmapped in the meanwhile?  Or, for that matter,
if that has been followed by mmap() of something with VM_IO on the
same range of addresses...  ->open() does *NOT* prevent any of that.