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 for Android: free password hash cracker in your pocket

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

Date:	Tue, 17 Apr 2012 13:29:25 +0100
From:	Mel Gorman <mgorman@...e.de>
To:	Martin Schwidefsky <schwidefsky@...ibm.com>
Cc:	Heiko Carstens <heiko.carstens@...ibm.com>,
	Hugh Dickins <hughd@...gle.com>,
	Rik van Riel <riel@...hat.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 Mon, Apr 16, 2012 at 05:50:40PM +0200, Martin Schwidefsky wrote:
> On Mon, 16 Apr 2012 15:14:23 +0100
> Mel Gorman <mgorman@...e.de> 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. 
> > 
> > However, s390 needs a better way of guarding against
> > PageSwapCache pages being removed from the radix tree while set_page_dirty()
> > is being called. The patch would be marginally better if in the PageSwapCache
> > case we simply tried to lock once and in the contended case just fail to
> > propogate the storage key. I lack familiarity with the s390 architecture
> > to be certain if this is safe or not. Suggestions on a better fix?
> 
> One though that crossed my mind is that maybe a better approach would be
> to move the page_test_and_clear_dirty check out of page_remove_rmap.
> What we need to look out for are code sequences of the form:
> 
> 	if (pte_dirty(pte))
> 		set_page_dirty(page);
> 	...
> 	page_remove_rmap(page);
> 
> There are four of those as far as I can see: in try_to_unmap_one,
> try_to_unmap_cluster, zap_pte, and zap_pte_range.
> 
> A valid implementation for s390 would be to test and clear the changed
> bit in the storage key for every of those pte_dirty() calls.
> 
> 	if (pte_dirty(pte) || page_test_and_clear_dirty(page))
> 		set_page_dirty(page);
> 	...
> 	page_remove_rmap(page); /* w/o page_test_clear_dirty */
> 

In the zap_pte_range() case at least, pte_dirty() is only being checked
for !PageAnon pages so if we took this approach we would miss
PageSwapCache pages. If we added the check then the same problem is hit
and we'd need additional logic there for s390 to drop the PTL, take the
page lock and retry the operation. It'd still be ugly :(

-- 
Mel Gorman
SUSE Labs
--
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/