linux-kernel - Re: mm: NULL ptr deref in balance_dirty_pages

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] [day] [month] [year] [list]

Message-ID: <20140226154736.GV9987@twins.programming.kicks-ass.net>
Date:	Wed, 26 Feb 2014 16:47:36 +0100
From:	Peter Zijlstra <peterz@...radead.org>
To:	"Kirill A. Shutemov" <kirill@...temov.name>
Cc:	Bob Liu <lliubbo@...il.com>, Sasha Levin <sasha.levin@...cle.com>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Subject: Re: mm: NULL ptr deref in balance_dirty_pages_ratelimited

On Wed, Feb 26, 2014 at 05:20:51PM +0200, Kirill A. Shutemov wrote:
> On Wed, Feb 26, 2014 at 10:48:30PM +0800, Bob Liu wrote:
> > > Do you relay on unlock_page() to have a compiler barrier?
> > >
> > 
> > Before your commit mapping is a local variable and be assigned before
> > unlock_page():
> > struct address_space *mapping = page->mapping;
> > unlock_page(dirty_page);
> > put_page(dirty_page);
> > if ((dirtied || page_mkwrite) && mapping) {
> > 
> > 
> > I'm afraid now "fault_page->mapping" might be changed to NULL after
> > "if ((dirtied || vma->vm_ops->page_mkwrite) && fault_page->mapping) {"
> > and then passed down to balance_dirty_pages_ratelimited(NULL).
> 
> I see what you try to fix. I wounder if we need to do
> 
> mapping = ACCESS_ONCE(fault_page->mapping);
> 
> instead.
> 
> The question is if compiler on its own can eliminate intermediate variable
> and dereference fault_page->mapping twice, as code with my patch does.
> I ask because smp_mb__after_clear_bit() in unlock_page() does nothing on
> some architectures.

That's a bug, and I have patches for that. That said; this is only ia64
and sparc32. ia64 has an actual full memory barrier in there very much
including a compiler fence. And sparc32 atomics do too.

In general, any atomic RMW op also implies a compiler fence. This
includes clear_bit().

That said; unlock_page() should have RELEASE semantics, this too
enforces that the read of page->mapping stay before the unlock_page().
The second usage of mapping may leak into the locked region, but it may
not re-read after.
--
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/