linux-kernel - Re: zone_reclaimable() leads to livelock in __alloc_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] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 23 May 2016 17:14:19 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Michal Hocko <mhocko@...nel.org>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Andrea Arcangeli <aarcange@...hat.com>,
	Mel Gorman <mgorman@...hsingularity.net>,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: zone_reclaimable() leads to livelock in __alloc_pages_slowpath()

On 05/23, Michal Hocko wrote:
>
> > 	nr_scanned = zone_page_state(zone, NR_PAGES_SCANNED);
> > 	if (nr_scanned)
> > 		__mod_zone_page_state(zone, NR_PAGES_SCANNED, -nr_scanned);
> >
> > and this doesn't look exactly right: zone_page_state() ignores the per-cpu
> > ->vm_stat_diff[] counters (and we probably do not want for_each_online_cpu()
> > loop here). And I do not know if this is really bad or not, but note that if
> > I change calculate_normal_threshold() to return 0, the problem goes away too.
>
> You are absolutely right that this is racy. In the worst case we would
> end up missing nr_cpus*threshold scanned pages which would stay behind.

and the sum of ->vm_diff[] can be negative, so...

> But
>
> bool zone_reclaimable(struct zone *zone)
> {
> 	return zone_page_state_snapshot(zone, NR_PAGES_SCANNED) <
> 		zone_reclaimable_pages(zone) * 6;
> }
>
> So the left over shouldn't cause it to return true all the time.

well if NR_PAGES_SCANNED doesn't grow enough it can even stay negative,
but zone_page_state_snapshot() returns zero in this case. In any case
we can underestimate zone_page_state_snapshot(NR_PAGES_SCANNED).

> In
> fact it could prematurely say false, right? (note that _snapshot variant
> considers per-cpu diffs [1]).

exactly because _snapshot() doesn't ignore the per-cpu counters.

> That being said I am not really sure why would the 0 threshold help for
> your test case.

Neither me. Except, of course, threshold==0 means the the code above will
work correctly. But I do not think this was the root of the problem.

> Could you add some tracing and see what are the numbers
> above?

with the patch below I can press Ctrl-C when it hangs, this breaks the
endless loop and the output looks like

	vmscan: ZONE=ffffffff8189f180 0 scanned=0 pages=6
	vmscan: ZONE=ffffffff8189eb00 0 scanned=1 pages=0
	...
	vmscan: ZONE=ffffffff8189eb00 0 scanned=2 pages=1
	vmscan: ZONE=ffffffff8189f180 0 scanned=4 pages=6
	...
	vmscan: ZONE=ffffffff8189f180 0 scanned=4 pages=6
	vmscan: ZONE=ffffffff8189f180 0 scanned=4 pages=6

the numbers are always small.

> [1] I am not really sure which kernel version have you tested - your
> config says 4.6.0-rc7 but this is true since 0db2cb8da89d ("mm, vmscan:
> make zone_reclaimable_pages more precise") which is 4.6-rc1.

Yes, I am on c5114626f33b62fa7595e57d87f33d9d1f8298a2, it has this change.

Oleg.

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 142cb61..6d221f9 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2614,6 +2614,12 @@ static bool shrink_zones(struct zonelist *zonelist, struct scan_control *sc)
 		if (shrink_zone(zone, sc, zone_idx(zone) == classzone_idx))
 			reclaimable = true;
 
+if (fatal_signal_pending(current))
+	pr_crit("ZONE=%p %d scanned=%ld pages=%ld\n",
+		zone, reclaimable,
+		zone_page_state_snapshot(zone, NR_PAGES_SCANNED),
+		zone_reclaimable_pages(zone));
+else
 		if (global_reclaim(sc) &&
 		    !reclaimable && zone_reclaimable(zone))
 			reclaimable = true;