<div>Thanks for your comments.</div><div> </div><div>Thank,</div><div>Inki Dae<br><br></div><div class="gmail_quote">2013/9/26 Al Viro <span dir="ltr"><<a href="mailto:viro@zeniv.linux.org.uk" target="_blank">viro@zeniv.linux.org.uk</a>></span><br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div class="im">On Tue, Sep 24, 2013 at 01:41:00PM +0900, Inki Dae wrote:<br>

<br>
> I can't see to hold ->mmap_sem when it calls find_vma() anywhere else.<br>
<br>
</div>Er...  What, in your opinion, would protect the result of find_vma(), if<br>
not that?  E.g. if another thread does munmap() on that area... </blockquote><div> </div><div>Right. the vm_area_struct object could be removed from rb tree; mm->mm_rb->rb_node.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid">
 vma isn't<br>
refcounted; there are only two things that can prevent its freeing -<br>
mmap_sem being held and the lack of anybody else seeing the address of<br>
mm_struct it belongs to (basically, when we are killing mm_struct off<br>
or when we are populating a fresh mm_struct; in the latter case the<br>
parent is locked, but the child doesn't need to).<br></blockquote><div> </div><div>Ok, will add down_write(&mm->mmap_sem) and up_write(&mm->mmap_sem) between and after find_vma() call.</div><div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid">
<br>
Look at page fault handlers - they grab mmap_sem (shared) before looking for<br>
vma.</blockquote><div> </div><div>Yes, and do_munmap also.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid">
  And anything modifying the list of vmas (vm_mmap(), etc.) grabs it<br>
exclusive...<br>
<div class="im"><br>
> > to caller) and clones it manually, regardless of whether that vma allows<br>
> > to clone itself or not.  Quite a few drivers rely on that not happening...<br>
> ><br>
><br>
> I think that has no any problem because exynos_gem_get_vma() takes a<br>
> reference to vma, and also v4l2 side is using same way. I and v4l2 guys<br>
> might be missing something what you are concerning. So Could you give us<br>
> more comments?<br>
<br>
</div>vb2_get_vma()/vb2_put_userptr() is also buggy.  At the very least, it<br>
should refuse to handle ones with VM_DONTCOPY in flags.  Again, there<br>
are drivers that seriously rely on VM_DONTCOPY being honoured.<br></blockquote><div> </div><div>Got it. will check the VM_DONTCOPY flag before copying the vma.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid">

<br>
BTW, what do you expect exynos_gem_get_pages_from_userptr() to do if<br>
the area has been unmapped in the meanwhile?</blockquote><div> </div><div>Yes, that function would try to access a invaild vma.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid">
  Or, for that matter,<br>
if that has been followed by mmap() of something with VM_IO on the<br>
same range of addresses...  ->open() does *NOT* prevent any of that.<br>
<div class="HOEnZb"><div class="h5">_______________________________________________<br>
dri-devel mailing list<br>
<a href="mailto:dri-devel@lists.freedesktop.org">dri-devel@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/dri-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/dri-devel</a><br>
</div></div></blockquote></div><br>