linux-kernel - Re: LTP regressions due to 6dc296e7df4c ("mm: make sure all file VMAs have ->vm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150915121201.GA10104@redhat.com>
Date:	Tue, 15 Sep 2015 14:12:01 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	"Kirill A. Shutemov" <kirill@...temov.name>
Cc:	"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
	Will Deacon <will.deacon@....com>, hpa@...or.com,
	luto@...capital.net, dave.hansen@...ux.intel.com, mingo@...e.hu,
	minchan@...nel.org, tglx@...utronix.de, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org
Subject: Re: LTP regressions due to 6dc296e7df4c ("mm: make sure all file
	VMAs have ->vm_ops set")

On 09/14, Kirill A. Shutemov wrote:
>
> On Mon, Sep 14, 2015 at 07:05:47PM +0200, Oleg Nesterov wrote:
> > On 09/14, Kirill A. Shutemov wrote:
> > >
> > > Fix is below. I don't really like it, but I cannot find any better
> > > solution.
> >
> > Me too...
> >
> > But this change "documents" the nasty special "vm_file && !vm_ops" case, and
> > I am not sure how we can remove it later...
> >
> > So perhaps we should change vma_is_anonymous() back to check ->fault too,
> >
> > 	 static inline bool vma_is_anonymous(struct vm_area_struct *vma)
> > 	 {
> > 	-	return !vma->vm_ops;
> > 	+	return !vma->vm_ops || !vma->vm_ops->fault;
>
> No. This would give a lot false positives from drives which setup page
> tables upfront and don't use ->fault at all.

And? I mean, I am not sure I understand what exactly do you dislike.

Firstly, I still think that (in the long term) we should change them
to use .faul = no_fault() which just returns VM_FAULT_SIGBUS.

Until then I do not see why the change above can be really bad. The
VM_SHARED case is fine, do_anonymous_page() will return VM_FAULT_SIGBUS.

So afaics the only problem is that after the change above the private
mapping can silently get an anonymous page after (say) MADV_DONTNEED
instead of the nice SIGBUS from do_fault(). I agree, this is not good,
but see above.

Or I missed something else?

Let me repeat, I am not going to really argue, you understand this all
much better than me. But imho we should try to avoid the special case
added by your change as much as possible, in this sense the change above
looks "obviously better" at least as a short-term fix.

Whether we need to keep the vm_ops/fault check in __vma_link_rb() and
mmap_region() is another issue. But if we keep them, then I think we
should at least turn the !vma->vm_ops check in mmap_region into
WARN_ON() as well.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/