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

[Al Viro <viro@ZenIV.linux.org.uk>]

On Mon, Sep 23, 2013 at 04:49:30PM +0900, Inki Dae wrote:
> Hi,
> 
> > -----Original Message-----
> > From: Al Viro [mailto:viro@ftp.linux.org.uk] On Behalf Of Al Viro
> > Sent: Monday, September 23, 2013 6:29 AM
> > To: YoungJun Cho
> > Cc: dri-devel@lists.freedesktop.org; Inki Dae
> > Subject: [RFC] deadlock in "drm/exynos: fix wrong pointer access at vm
> > close"
> > 
> > 	You have drm_dev->struct_mutex grabbed before ->mmap_sem in
> > exynos_drm_gem_mmap_ioctl() and after - in exynos_drm_gem_fault()
> > (since ->fault() is always called with ->mmap_sem held).  Looks like
> > a garden-variety AB-BA deadlock...
> > 
> 
> Right, if mmap system call is requested by another process between ->f_op
> pointer changing and restoring, the deadlock can be incurred.
> 
> For this, I think we can resolve this issue like below,
> 
> At exynos_drm_gem_mmap_ioctl()
> 	down_write(&mm->mmap_sem);
> 	mutex_lock(&dev->struct_mutex);
> 	...

Umm...  If you do it that way, why bother with changing ->private_data
at all?  You can as well stash obj in dev->dev_private->something after
you've grabbed the mutex and have ->mmap() pick it there...

Said that, I really don't like that approach - both playing with ->f_op
and the games with private vmas; exynos_gem_get_vma(), AFAICS, calls
find_vma() (without bothering to hold ->mmap_sem, BTW - there's nothing
to prevent the result of find_vma() being freed just as it's returned
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...

IOW, you are already digging deep inside VM guts and this only makes
it deeper ;-/

And no, the deadlock doesn't depend on race between ioctl() and mmap()
from another process; all it takes is
1) thread A does clone(), creating thread B that shares address space with
it.
2) thread A does that ioctl, creating a mapping
3) thread A does that ioctl again, creating another mapping, while thread
B dereferences an address in the first mapping and triggers a page fault.

The only race is on step 3 in the above; the question about mmap() vs.
ioctl() was about mmap(2) _during_ that ioctl() hitting
exynos_drm_gem_mmap_buffer() instead of exynos_drm_gem_mmap() it would've
called before or after ioctl().  Here the interesting case is when
callers of mmap() and ioctl() do *not* share the address space, since
in that case mmap(2) won't notice ->mmap_sem held by you - it's on the
different mm_struct, so mmap(2) will get to calling the ->f_op->mmap()
without waiting for you to restore ->f_op...

For another bug in the same area, try building that driver modular and
watch what happens to use count after a round of this switching ->f_op and
restoring it back to original; fops_get() in there is wrong and AFAICS
pointless.