linux-kernel - Re: [PATCH 1/10] shmem: replace page if mapping excludes its zone

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: <alpine.LSU.2.00.1205141219340.1623@eggly.anvils>
Date:	Mon, 14 May 2012 12:42:21 -0700 (PDT)
From:	Hugh Dickins <hughd@...gle.com>
To:	Cong Wang <xiyou.wangcong@...il.com>
cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Christoph Hellwig <hch@...radead.org>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Alan Cox <alan@...rguk.ukuu.org.uk>,
	Stephane Marchesin <marcheu@...omium.org>,
	Andi Kleen <andi@...stfloor.org>,
	Dave Airlie <airlied@...il.com>,
	Daniel Vetter <ffwll.ch@...gle.com>,
	Rob Clark <rob.clark@...aro.org>, linux-mm@...ck.org,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/10] shmem: replace page if mapping excludes its zone

On Mon, 14 May 2012, Cong Wang wrote:
> On 05/12/2012 07:59 PM, Hugh Dickins wrote:
> > +	VM_BUG_ON(!PageLocked(oldpage));
> > +	__set_page_locked(newpage);
> > +	VM_BUG_ON(!PageUptodate(oldpage));
> > +	SetPageUptodate(newpage);
> > +	VM_BUG_ON(!PageSwapBacked(oldpage));
> > +	SetPageSwapBacked(newpage);
> > +	VM_BUG_ON(!swap_index);
> > +	set_page_private(newpage, swap_index);
> > +	VM_BUG_ON(!PageSwapCache(oldpage));
> > +	SetPageSwapCache(newpage);
> > +
> 
> Are all of these VM_BUG_ON's necessary?

I'm really glad you asked that - thank you.

At first I was just going to brush you off with a standard reply of
something like "well, no BUG_ON should ever be necessary, but we do
find them helpful in practice".

But (a) these ones have probably outlived their usefulness: they were
certainly reassuring to me when I was testing, but perhaps now are
just cluttering up the flow.  I did make them "VM_" BUG_ONs in the
hope that distros wouldn't waste space and time switching them on, but
now I'm inclined to agree with you that they should just be removed.
Most of them are doing no more than confirm what's been checked before
calling the function (and confirming that status cannot racily change).

And (b) whereas they didn't actually catch anything for me, they have
been giving false assurance: because (I believe) there really is a bug
lurking there that they have not yet met and caught.  And I would have
missed it if you hadn't directed me back to think about these.

It is an exceedingly unlikely bug (and need not delay use of the patch),
but what I'm re-remembering is just how slippery swap is: the problem is
that a swapcache page can get freed and reused before getting the page
lock on it; and it might even get reused for swapcache.  Perhaps I need
also to be checking page->private, or perhaps I need to check for error
instead of BUG_ON(error) just before the lines you picked out, or both.

I'm not going to rush the incremental patch to fix this: need to think
about it quietly first.

If you're wondering what I'm talking about (sorry, I don't have time
to explain more right now), take a look at comment and git history of
line 2956 (in 3.4-rc7) of mm/memory.c:
	if (unlikely(!PageSwapCache(page) || page_private(page) != entry.val))
I don't suppose anyone ever actually hit the bug in the years before
we added that protection, but we still ought to guard against it,
there and here in shmem_replace_page().

Hugh
--
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/