linux-kernel - Re: [BUG] infinite loop in find_get

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.1109141427190.29355@sister.anvils>
Date:	Wed, 14 Sep 2011 14:53:52 -0700 (PDT)
From:	Hugh Dickins <hughd@...gle.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
cc:	Shaohua Li <shaohua.li@...el.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Rik van Riel <riel@...hat.com>,
	Lin Ming <mlin@...pku.edu.cn>,
	Justin Piszcz <jpiszcz@...idpixels.com>,
	Pawel Sikora <pluto@...k.net>
Subject: Re: [BUG] infinite loop in find_get_pages()

On Wed, 14 Sep 2011, Eric Dumazet wrote:
> Le mercredi 14 septembre 2011 à 13:38 -0700, Hugh Dickins a écrit :
> > 
> > I'd like to think about that a little more before finalizing the
> > patch below - does it work, and does it look acceptable so far?
> > Of course, the mods to truncate.c and vmscan.c are not essential
> > parts of this fix, just things to tidy up while on the subject.
> > Right now I must attend to some other stuff, will return tomorrow.
> 
> Hello Hugh
> 
> I am going to test this ASAP,

Thanks, Eric, though it may not be worth spending your time on it.
It occurred to me over lunch that it may take painfully longer than
expected to invalidate_mapping_pages() on a single-swapped-out-page
1TB sparse tmpfs file - all those "start += 1" restarts until it
reaches the end.

I might decide to leave invalidate_mapping_pages() giving up early
(unsatisfying, but no worse than before), and convert scan_mapping_
unevictable_pages() (which is used on nothing but shmem) to pass
index vector to radix_tree_gang_whatever().

Dunno, I'll think about it more later.

> but have one question below :
> 
> >  	/*
> >  	 * If all entries were removed before we could secure them,
> >  	 * try again, because callers stop trying once 0 is returned.
> >  	 */
> > -	if (unlikely(!ret && nr_found))
> > +	if (unlikely(!ret && nr_found)) {
> > +		cond_resched();
> > +		start += nr_found;
> 
> Isnt it possible to go out of initial window ?
> start could be greater than 'end' ?
> 
> invalidate_mapping_pages()
> 
> does some capping (end - index)

Good question, but even before the change (or any of my changes here)
it's perfectly possible to go out of the initial window - the radix_tree
gang interfaces allow you to specify the maximum you want back (i.e. size
of buffer), but they do not actually allow you to specify end of range.

There's a few places where we trim the maximum to match our end of range,
but that's just a slight optimization in the face of an arguably incomplete
interface.  But the radix_tree is not too inefficient this way, because of
how empty nodes get removed immediately - there's a limit to the number
of nodes it will have to look through before it fills the buffer.

> 
> 
> >  	pagevec_init(&pvec, 0);
> >  	while (index <= end && pagevec_lookup(&pvec, mapping, index,
> >  			min(end - index, (pgoff_t)PAGEVEC_SIZE - 1) + 1)) {

It does cap by "end - index", but it has already checked "index <= end",
and it is only this minor optimization, nothing essential.

Hugh