linux-kernel - Re: [PATCH 2/7] mm: Rewrite shmem_seek_hole

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: <20200820190444.GN17456@casper.infradead.org>
Date:   Thu, 20 Aug 2020 20:04:44 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     Mike Rapoport <rppt@...nel.org>
Cc:     linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
        Hugh Dickins <hughd@...gle.com>,
        William Kucharski <william.kucharski@...cle.com>,
        Johannes Weiner <hannes@...xchg.org>, Jan Kara <jack@...e.cz>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/7] mm: Rewrite shmem_seek_hole_data

On Thu, Aug 20, 2020 at 07:45:46PM +0300, Mike Rapoport wrote:
> > - * llseek SEEK_DATA or SEEK_HOLE through the page cache.
> > + * llseek SEEK_DATA or SEEK_HOLE through the page cache.  We don't need
> > + * to get a reference on the page because this interface is racy anyway.
> > + * The page we find will have had the state at some point.
> 
> For my non-native ear "will have had" is too complex ;-)

Fair!  How about simply "The page we find did have the state at some point".

But now I'm thinking about it some more, and I'm not so sure of it now.
What if we put a page in the cache that was !Uptodate, then we do a
lookup, find this pointer, then the page is removed from the cache,
then it's allocated somewhere else, marked Uptodate, and now we're
scheduled back in and find it's Uptodate, when there was never a page
at this location that was Uptodate?

So maybe I have to retract this patch after all.

> >   */
> >  static pgoff_t shmem_seek_hole_data(struct address_space *mapping,
> >  				    pgoff_t index, pgoff_t end, int whence)
> >  {
> > +	XA_STATE(xas, &mapping->i_pages, index);
> >  	struct page *page;
> > -	struct pagevec pvec;
> > -	pgoff_t indices[PAGEVEC_SIZE];
> > -	bool done = false;
> > -	int i;
> >  
> > -	pagevec_init(&pvec);
> > -	pvec.nr = 1;		/* start small: we may be there already */
> > -	while (!done) {
> > -		pvec.nr = find_get_entries(mapping, index,
> > -					pvec.nr, pvec.pages, indices);
> > -		if (!pvec.nr) {
> > -			if (whence == SEEK_DATA)
> > -				index = end;
> > -			break;
> > +	rcu_read_lock();
> > +	if (whence == SEEK_DATA) {
> > +		for (;;) {
> > +			page = xas_find(&xas, end);
> > +			if (xas_retry(&xas, page))
> > +				continue;
> > +			if (!page || xa_is_value(page) || PageUptodate(page))
> > +				break;
> >  		}
> > -		for (i = 0; i < pvec.nr; i++, index++) {
> > -			if (index < indices[i]) {
> > -				if (whence == SEEK_HOLE) {
> > -					done = true;
> > -					break;
> > -				}
> > -				index = indices[i];
> > -			}
> > -			page = pvec.pages[i];
> > -			if (page && !xa_is_value(page)) {
> > -				if (!PageUptodate(page))
> > -					page = NULL;
> > -			}
> > -			if (index >= end ||
> > -			    (page && whence == SEEK_DATA) ||
> > -			    (!page && whence == SEEK_HOLE)) {
> > -				done = true;
> > +	} else /* SEEK_HOLE */ {
> > +		for (;;) {
> > +			page = xas_next(&xas);
> > +			if (xas_retry(&xas, page))
> > +				continue;
> > +			if (!xa_is_value(page) &&
> > +					(!page || !PageUptodate(page)))
> > +				break;
> > +			if (xas.xa_index >= end)
> >  				break;
> > -			}
> >  		}
> > -		pagevec_remove_exceptionals(&pvec);
> > -		pagevec_release(&pvec);
> > -		pvec.nr = PAGEVEC_SIZE;
> > -		cond_resched();
> >  	}
> > -	return index;
> > +	rcu_read_unlock();
> > +
> > +	return xas.xa_index;
> >  }
> >  
> >  static loff_t shmem_file_llseek(struct file *file, loff_t offset, int whence)
> > -- 
> > 2.28.0
> > 
> > 
> 
> -- 
> Sincerely yours,
> Mike.