On Tue, Aug 29, 2017 at 03:18:25PM +0200, Laurent Dufour wrote: > On 29/08/2017 14:04, Peter Zijlstra wrote: > > On Tue, Aug 29, 2017 at 09:59:30AM +0200, Laurent Dufour wrote: > >> On 27/08/2017 02:18, Kirill A. Shutemov wrote: > >>>> + > >>>> + if (unlikely(!vma->anon_vma)) > >>>> + goto unlock; > >>> > >>> It deserves a comment. > >> > >> You're right I'll add it in the next version. > >> For the record, the root cause is that __anon_vma_prepare() requires the > >> mmap_sem to be held because vm_next and vm_prev must be safe. > > > > But should that test not be: > > > > if (unlikely(vma_is_anonymous(vma) && !vma->anon_vma)) > > goto unlock; > > > > Because !anon vmas will never have ->anon_vma set and you don't want to > > exclude those. > > Yes in the case we later allow non anonymous vmas to be handled. > Currently only anonymous vmas are supported so the check is good enough, > isn't it ?
That wasn't at all clear from reading the code. This makes it clear ->anon_vma is only ever looked at for anonymous. And like Kirill says, we _really_ should start allowing some (if not all) vm_ops. Large file based mappings aren't particularly rare. I'm not sure we want to introduce a white-list or just bite the bullet and audit all ->fault() implementations. But either works and isn't terribly difficult, auditing all is more work though.
