Re: [PATCH v2 14/20] mm: Provide speculative fault infrastructure

On Tue, Aug 29, 2017 at 03:18:25PM +0200, Laurent Dufour wrote:

On 29/08/2017 14:04, Peter Zijlstra wrote:

quoted

On Tue, Aug 29, 2017 at 09:59:30AM +0200, Laurent Dufour wrote:

quoted

On 27/08/2017 02:18, Kirill A. Shutemov wrote:

quoted

quoted

+
+	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.