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...
Incidentally, what should happen if another process shares the same opened file (e.g. inherited over fork()) and does mmap() just as we have ->f_op switched?
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); ... addr = security_mmap_file(...); if (!addr) { unsigned long populate;
addr = do_mmap_pgoff(...); if (populate) mm_populater(...); } ... mutex_unlock(&dev->struct_mutex); up_write(&mm->mmap_sem);
The point above is to call down_write(&mm->mmap_sem) before do_mmap_pgoff, and then to call up_write(...) after do_mmap_pg_off so that when another process called mmap system call, the process can wait for up_write(...) until the ->f_op pointer is restored.
I will post this patch soon. other opinions?
Thanks, Inki Dae
Incidentally, what should happen if another process shares the same opened file (e.g. inherited over fork()) and does mmap() just as we have ->f_op switched?
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.
Thanks for your comment.
-----Original Message----- From: Al Viro [mailto:viro@ftp.linux.org.uk] On Behalf Of Al Viro Sent: Monday, September 23, 2013 10:07 PM To: Inki Dae Cc: 'YoungJun Cho'; dri-devel@lists.freedesktop.org Subject: Re: [RFC] deadlock in "drm/exynos: fix wrong pointer access at vm close"
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...
I changed ->private_data to a gem object so that mmap_buffer function can get the gem object. But yes, we can use dev->dev_private->something instead, and that seems like better way. For this, I missed that we can get drm_device object from file pointer.
Said that, I really don't like that approach - both playing with ->f_op and the games with private vmas;
Agree. But is there other better way to support direct mapping; not needing page fault handler to map, and indirect mapping interfaces; needing the page fault handler?
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
I can't see to hold ->mmap_sem when it calls find_vma() anywhere else.
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?
And, I found a bug that we never call exynos_gem_put_vma() when userptr is freed. That should be fixed.
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
Yes, there was my wrong comments. That is a mmap callback(exynos_drm_gem_mmap_buffer) we don't want can be called if we called mmap system call when ->f_op points to exynos_drm_gem_fops.
- 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...
Yes, it's a problem. When two more threads are running, and when thread A called mmap system call, exynos_drm_gem_mmap_buffer() will be called if thread B called ioctl() first and f_op was not restored yet. I have no idea to resolve this issue yet. So do you have any good idea? At any rate, we should fix this issue.
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.
Yes, it would also be another bug. Will check it out. It seems that it's not good way to change and restore ->f_op, and this way makes mmap part to be complicated. I will try to find a way to resolve this issue or to find another better way to support direct and indirect mapping interfaces.
I'd appreciate all the advices I can get.
Thanks, Inki Dae
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.
Thanks for your comments.
Thank, Inki Dae
2013/9/26 Al Viro viro@zeniv.linux.org.uk
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...
Right. the vm_area_struct object could be removed from rb tree; mm->mm_rb->rb_node.
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).
Ok, will add down_write(&mm->mmap_sem) and up_write(&mm->mmap_sem) between and after find_vma() call.
Look at page fault handlers - they grab mmap_sem (shared) before looking for vma.
Yes, and do_munmap also.
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.
Got it. will check the VM_DONTCOPY flag before copying the vma.
BTW, what do you expect exynos_gem_get_pages_from_userptr() to do if the area has been unmapped in the meanwhile?
Yes, that function would try to access a invaild vma.
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. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
-----Original Message----- From: Inki Dae [mailto:inki.dae@samsung.com] Sent: Tuesday, September 24, 2013 1:41 PM To: 'Al Viro' Cc: 'YoungJun Cho'; 'dri-devel@lists.freedesktop.org' Subject: RE: [RFC] deadlock in "drm/exynos: fix wrong pointer access at vm close"
Thanks for your comment.
-----Original Message----- From: Al Viro [mailto:viro@ftp.linux.org.uk] On Behalf Of Al Viro Sent: Monday, September 23, 2013 10:07 PM To: Inki Dae Cc: 'YoungJun Cho'; dri-devel@lists.freedesktop.org Subject: Re: [RFC] deadlock in "drm/exynos: fix wrong pointer access at
vm
close"
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...
I changed ->private_data to a gem object so that mmap_buffer function can get the gem object. But yes, we can use dev->dev_private->something instead, and that seems like better way. For this, I missed that we can get drm_device object from file pointer.
Said that, I really don't like that approach - both playing with ->f_op and the games with private vmas;
Agree. But is there other better way to support direct mapping; not needing page fault handler to map, and indirect mapping interfaces; needing the page fault handler?
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
I can't see to hold ->mmap_sem when it calls find_vma() anywhere else.
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?
And, I found a bug that we never call exynos_gem_put_vma() when userptr is freed. That should be fixed.
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
Yes, there was my wrong comments. That is a mmap callback(exynos_drm_gem_mmap_buffer) we don't want can be called if we called mmap system call when ->f_op points to exynos_drm_gem_fops.
- 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...
Yes, it's a problem. When two more threads are running, and when thread A called mmap system call, exynos_drm_gem_mmap_buffer() will be called if thread B called ioctl() first and f_op was not restored yet. I have no idea to resolve this issue yet. So do you have any good idea? At any rate, we should fix this issue.
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.
Yes, it would also be another bug. Will check it out. It seems that it's not good way to change and restore ->f_op, and this way makes mmap part to be complicated. I will try to find a way to resolve this issue or to find another better way to support direct and indirect mapping interfaces.
It seems that we can use a new anon file instead of using drm file to resolve the issue.
Thanks, Inki Dae
I'd appreciate all the advices I can get.
Thanks, Inki Dae
On Wed, Sep 25, 2013 at 01:34:30PM +0900, Inki Dae wrote:
It seems that we can use a new anon file instead of using drm file to resolve the issue.
Could you describe what are you trying to achieve with that ioctl() and what semantics do you want from normal mmap()?
2013/9/26 Al Viro viro@zeniv.linux.org.uk
On Wed, Sep 25, 2013 at 01:34:30PM +0900, Inki Dae wrote:
It seems that we can use a new anon file instead of using drm file to resolve the issue.
Could you describe what are you trying to achieve with that ioctl() and what semantics do you want from normal mmap()?
Ok, The reason that we are using ioctl mmap is that I thought it doesn't need demand paging mechanism for drm driver for ARM SoCs: they have no iommu so need physically continuous memory for DMA.
And another reason is that dumb* interface - it seems that you say this interface is a normal mmap but this is a fake mmap - has more steps against ioctl mmap. So to map some user space with physical pages, first, application should request a gem allocation, and then request a hash key of the allcated gem, and then fake mmap. This fake mmap system call doesn't map a given user space with the physical pages. And then the user space will be mapped with the physical pages by page fault handler when cpu tries to access the user space. On the other hand, the ioctl mmap call will map the user space with physical pages, and I thought this way would have better performance than normal mmap at least in case of ARM based SoC.
And last, the reason I had implemented the normal mmap is that dumb* interfaces are required as default. i.e. modetest app of libdrm uses only dumb interfaces for buffer management.
Anyway,we have faced with some issues by using ioctl mmap and normal mmap together, and these issues should be fixed.
Thanks, Inki Dae
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
2013/9/29 Inki Dae inki.dae@samsung.com
2013/9/26 Al Viro viro@zeniv.linux.org.uk
On Wed, Sep 25, 2013 at 01:34:30PM +0900, Inki Dae wrote:
It seems that we can use a new anon file instead of using drm file to resolve the issue.
Could you describe what are you trying to achieve with that ioctl() and what semantics do you want from normal mmap()?
Ok, The reason that we are using ioctl mmap is that I thought it doesn't need demand paging mechanism for drm driver for ARM SoCs: they have no iommu so need physically continuous memory for DMA.
Sorry, there was a wrong comment. it's not all ARM SoC have no iommu. Exynos also has the iommu, and maybe recently most SoCs.
And another reason is that dumb* interface - it seems that you say this interface is a normal mmap but this is a fake mmap - has more steps against ioctl mmap. So to map some user space with physical pages, first, application should request a gem allocation, and then request a hash key of the allcated gem, and then fake mmap. This fake mmap system call doesn't map a given user space with the physical pages. And then the user space will be mapped with the physical pages by page fault handler when cpu tries to access the user space. On the other hand, the ioctl mmap call will map the user space with physical pages, and I thought this way would have better performance than normal mmap at least in case of ARM based SoC.
And last, the reason I had implemented the normal mmap is that dumb* interfaces are required as default. i.e. modetest app of libdrm uses only dumb interfaces for buffer management.
Anyway,we have faced with some issues by using ioctl mmap and normal mmap together, and these issues should be fixed.
Thanks, Inki Dae
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Al Viro -
Inki Dae