On Mon, Sep 23, 2013 at 04:49:30PM +0900, Inki Dae wrote: > Hi, > > > -----Original Message----- > > From: Al Viro [mailto:viro@xxxxxxxxxxxxxxxx] On Behalf Of Al Viro > > Sent: Monday, September 23, 2013 6:29 AM > > To: YoungJun Cho > > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; 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. _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
