linux-kernel - Re: Questin about swap_slot free and invalidate page

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]

Message-ID: <alpine.LNX.2.00.1302031732520.4050@eggly.anvils>
Date:	Sun, 3 Feb 2013 17:51:14 -0800 (PST)
From:	Hugh Dickins <hughd@...gle.com>
To:	Minchan Kim <minchan@...nel.org>
cc:	Nitin Gupta <ngupta@...are.org>,
	Dan Magenheimer <dan.magenheimer@...cle.com>,
	Seth Jennings <sjenning@...ux.vnet.ibm.com>,
	Konrad Rzeszutek Wilk <konrad@...nok.org>,
	linux-mm@...ck.org, linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: Questin about swap_slot free and invalidate page

On Thu, 31 Jan 2013, Minchan Kim wrote:

> When I reviewed zswap, I was curious about frontswap_store.
> It said following as.
> 
>  * If frontswap already contains a page with matching swaptype and
>  * offset, the frontswap implementation may either overwrite the data and
>  * return success or invalidate the page from frontswap and return failure.
> 
> It didn't say why it happens. we already have __frontswap_invalidate_page
> and call it whenever swap_slot frees. If we don't free swap slot,
> scan_swap_map can't find the slot for swap out so I thought overwriting of
> data shouldn't happen in frontswap.
> 
> As I looked the code, the curplit is reuse_swap_page. It couldn't free swap
> slot if the page founded is PG_writeback but miss calling frontswap_invalidate_page
> so data overwriting on frontswap can happen. I'm not sure frontswap guys
> already discussed it long time ago.
> 
> If we can fix it, we can remove duplication entry handling logic
> in all of backend of frontswap. All of backend should handle it although
> it's pretty rare. Of course, zram could be fixed. It might be trivial now
> but more there are many backend of frontswap, more it would be a headache.
> 
> If we are trying to fix it in swap layer,  we might fix it following as
> 
> int reuse_swap_page(struct page *page)
> {
>         ..
>         ..
>         if (count == 1) {
>                 if (!PageWriteback(page)) {
>                         delete_from_swap_cache(page);
>                         SetPageDirty(page);
>                 } else {
>                         frontswap_invalidate_page();
>                         swap_slot_free_notify();
>                 }
>         }
> }
> 
> But not sure, it is worth at the moment and there might be other places
> to be fixed.(I hope Hugh can point out if we are missing something if he
> has a time)

I expect you are right that reuse_swap_page() is the only way it would
happen for frontswap; but I'm too unfamiliar with frontswap to promise
you that - it's better that you insert WARN_ONs in your testing to verify.

But I think it's a general tmem property, isn't it?  To define what
happens if you do give it the same key again.  So I doubt it's something
that has to be fixed; but if you do find it helpful to fix it, bear in
mind that reuse_swap_page() is an odd corner, which may one day give the
"stable pages" DIF/DIX people trouble, though they've not yet complained.

I'd prefer a patch not specific to frontswap, but along the lines below:
I think that's the most robust way to express it, though I don't think
the (count == 0) case can actually occur inside that block (whereas
count == 0 certainly can occur in the !PageSwapCache case).

I believe that I once upon a time took statistics of how often the
PageWriteback case happens here, and concluded that it wasn't often
enough that refusing to reuse in this case would be likely to slow
anyone down noticeably.

> 
> If we are reluctant to it, at least, we should write out comment above
> frontswap_store about that to notice curious guys who spend many
> time to know WHY and smart guys who are going to fix it with nice way.
> 
> Mr. Frontswap, What do you think about it?

He's not me of course :)

Hugh

--- 3.8-rc6/mm/swapfile.c	2012-12-22 09:43:27.668015583 -0800
+++ linux/mm/swapfile.c	2013-02-03 17:31:04.148181857 -0800
@@ -637,8 +637,11 @@ int reuse_swap_page(struct page *page)
 		return 0;
 	count = page_mapcount(page);
 	if (count <= 1 && PageSwapCache(page)) {
-		count += page_swapcount(page);
-		if (count == 1 && !PageWriteback(page)) {
+		if (PageWriteback(page))
+			count = 2;	/* not safe yet to free its swap */
+		else
+			count += page_swapcount(page);
+		if (count <= 1) {
 			delete_from_swap_cache(page);
 			SetPageDirty(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/