linux-kernel - Re: [RFC PATCH] s390: mm: rmap: Transfer storage key to struct page under the page lock

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.1204172023390.1609@eggly.anvils>
Date:	Tue, 17 Apr 2012 20:52:21 -0700 (PDT)
From:	Hugh Dickins <hughd@...gle.com>
To:	Mel Gorman <mgorman@...e.de>
cc:	Martin Schwidefsky <schwidefsky@...ibm.com>,
	Heiko Carstens <heiko.carstens@...ibm.com>,
	Rik van Riel <riel@...hat.com>,
	Ken Chen <kenchen@...gle.com>, Linux-MM <linux-mm@...ck.org>,
	Linux-S390 <linux-s390@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH] s390: mm: rmap: Transfer storage key to struct page
 under the page lock

On Tue, 17 Apr 2012, Mel Gorman wrote:
> On Mon, Apr 16, 2012 at 02:14:09PM -0700, Hugh Dickins wrote:
> > On Mon, 16 Apr 2012, Mel Gorman wrote:
> > 
> > > This patch is horribly ugly and there has to be a better way of doing
> > > it. I'm looking for suggestions on what s390 can do here that is not
> > > painful or broken. 
> > > 
> > > The following bug was reported on s390
> > > 
> > > kernel BUG at
> > > /usr/src/packages/BUILD/kernel-default-3.0.13/linux-3.0/lib/radix-tree.c:477!
>... 
> > I'm confused as to whether you see this problem with file pages,
> > or with anon-swap-cache pages, or with both, or not yet determined.
> 
> PageSwapCache pages only.

Oh good, thanks, that narrowed the search space a lot.

> > (You do remind me that I meant years ago to switch swapper_space over
> > to the much simpler __set_page_dirty_no_writeback(), which shmem has
> > used for ages; but as far as this problem goes, that would probably
> > be at best a workaround, rather than the proper fix.)
> 
> It would be a workaround. If in the future we wanted to treat swapper
> space more like a normal file inode and writeback dirty pages from
> the flusher thread then this bug would just pop its head back up.

It's a no-brainer workaround: patch and more explanation below.  I
can double-fix it if you prefer, but the one-liner appeals more to me.

> > Hmm, mm/migrate.c.
> 
> Migration moves the page mapping under the tree lock so
> __set_page_dirty_nobuffers() I don't think that is it.

Yes, I was worried by the places that set page->mapping = NULL in
migrate.c (later, not under the tree_lock), but those would not be able
to generate this issue at all (ptes already replaced by migration entries).

> I think the race is against something like reuse_swap_page() which locks
> the page and removes it from swap cache while page_remove_rmap() looks
> up the same page.

No, __delete_from_swap_cache() is always doing ClearPageSwapCache under
tree_lock (which __set_page_dirty_no_buffers acquires before proceeding).

[PATCH] mm: fix s390 BUG by using __set_page_dirty_no_writeback on swap

Mel reports a BUG_ON(slot == NULL) in radix_tree_tag_set() on s390 3.0.13:
called from __set_page_dirty_nobuffers() when page_remove_rmap() tries to
transfer dirty flag from s390 storage key to struct page and radix_tree.

That would be because of reclaim's shrink_page_list() calling add_to_swap()
on this page at the same time: first PageSwapCache is set (causing
page_mapping(page) to appear as &swapper_space), then page->private set,
then tree_lock taken, then page inserted into radix_tree - so there's
an interval before taking the lock when the radix_tree slot is empty.

We could fix this by moving __add_to_swap_cache()'s spin_lock_irq up
before SetPageSwapCache, with error case ClearPageSwapCache moved up
under tree_lock too.

But a better fix is just to do what's five years overdue.  Ken Chen
added __set_page_dirty_no_writeback() (if !PageDirty TestSetPageDirty)
for tmpfs to skip all that radix_tree overhead, and swap is just the same:
it ignores the radix_tree tag, and does not participate in dirty page
accounting, so should be using __set_page_dirty_no_writeback() too.

Reported-by: Mel Gorman <mgorman@...e.de>
Signed-off-by: Hugh Dickins <hughd@...gle.com>
Cc: stable@...nel.org
---

 mm/swap_state.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- 3.4-rc2/mm/swap_state.c	2012-03-31 17:42:26.949729938 -0700
+++ linux/mm/swap_state.c	2012-04-17 15:34:05.732086663 -0700
@@ -26,7 +26,7 @@
  */
 static const struct address_space_operations swap_aops = {
 	.writepage	= swap_writepage,
-	.set_page_dirty	= __set_page_dirty_nobuffers,
+	.set_page_dirty	= __set_page_dirty_no_writeback,
 	.migratepage	= migrate_page,
 };

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