linux-kernel - Re: [RFC v2 PATCH] fs: __generic_file_splice_read retry lookup on AOP_TRUNCATED

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] [day] [month] [year] [list]

Message-ID: <20151216164745.GA29214@quack.suse.cz>
Date:	Wed, 16 Dec 2015 17:47:45 +0100
From:	Jan Kara <jack@...e.cz>
To:	Abhijith Das <adas@...hat.com>
Cc:	Jan Kara <jack@...e.cz>, linux-kernel@...r.kernel.org,
	linux-fsdevel@...r.kernel.org, Bob Peterson <rpeterso@...hat.com>
Subject: Re: [RFC v2 PATCH] fs: __generic_file_splice_read retry lookup on
 AOP_TRUNCATED_PAGE

On Wed 16-12-15 10:55:25, Abhijith Das wrote:
> > > @@ -439,14 +441,13 @@ __generic_file_splice_read(struct file *in, loff_t
> > > *ppos,
> > >  			error = mapping->a_ops->readpage(in, page);
> > >  			if (unlikely(error)) {
> > >  				/*
> > > -				 * We really should re-lookup the page here,
> > > -				 * but it complicates things a lot. Instead
> > > -				 * lets just do what we already stored, and
> > > -				 * we'll get it the next time we are called.
> > > +				 * Re-lookup the page
> > >  				 */
> > > -				if (error == AOP_TRUNCATED_PAGE)
> > > +				if (error == AOP_TRUNCATED_PAGE) {
> > >  					error = 0;
> > > -
> > > +					if (retries++ < 3)
> > > +						goto retry_lookup;
> > > +				}
> > 
> > I don't like this retry-three-times loop. That is still leaving the
> > possibility of 0 return just much less likely (so it will lead to even
> > weirder and harded to debug failures). IMO we should just terminate the
> > loop like we did previously if spd.nr_pages > 0 and we retry indefinitely
> > if it is the first page that failed to read.
> 
> The 3-retry loop was the thing I was unsure about too. With regards to the
> indefinite retry, I was wondering if there's some corner case where we might
> get into an infinite retry loop...

Well you have two options:

1) Return incorrect value from splice_read()
2) Retry indefinitely

Option two looks better to me. Also do_generic_file_read() behaves the same
way so mirroring the behavior in __generic_file_splice_read() makes sense.

> If we're doing the retry for the first page, why not for other pages too?
> Is it because we'd potentially be increasing the odds for an infinite
> loop and/or affecting performance by doing more lookups?

Yes, that was my thought. But seeing now that do_generic_file_read()
actually retries indefinitely for every page, I'd just do the same in 
__generic_file_splice_read().

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR
--
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/