linux-kernel - Re: [RFC v3 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]

Date:	Thu, 17 Dec 2015 20:23:09 +0100
From:	Jan Kara <jack@...e.cz>
To:	Abhi Das <adas@...hat.com>
Cc:	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	Bob Peterson <rpeterso@...hat.com>, Jan Kara <jack@...e.com>
Subject: Re: [RFC v3 PATCH] fs: __generic_file_splice_read retry lookup on
 AOP_TRUNCATED_PAGE

On Wed 16-12-15 13:02:52, Abhi Das wrote:
> During testing, I discovered that __generic_file_splice_read() returns
> 0 (EOF) when aops->readpage fails with AOP_TRUNCATED_PAGE on the first
> page of a single/multi-page splice read operation. This EOF return code
> causes the userspace test to (correctly) report a zero-length read error
> when it was expecting otherwise.
> 
> The current strategy of returning a partial non-zero read when ->readpage
> returns AOP_TRUNCATED_PAGE works only when the failed page is not the
> first of the lot being processed.
> 
> This patch attempts to retry lookup and call ->readpage again on pages
> that had previously failed with AOP_TRUNCATED_PAGE. With this patch, my
> tests pass and I haven't noticed any unwanted side effects.
> 
> This version removes the thrice-retry loop and instead indefinitely
> retries lookups on AOP_TRUNCATED_PAGE errors from ->readpage.
> 
> Signed-off-by: Abhi Das <adas@...hat.com>
> Cc: Bob Peterson <rpeterso@...hat.com>
> Cc: Jan Kara <jack@...e.com>

The patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@...e.cz>

BTW, you should send the patch to Al Viro as he is the VFS maintainer so he
should merge the patch. It might be also useful to mention in the changelog
that do_generic_file_read() behaves the same way.

								Honza
> ---
>  fs/splice.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/splice.c b/fs/splice.c
> index 801c21c..277df71 100644
> --- a/fs/splice.c
> +++ b/fs/splice.c
> @@ -415,6 +415,7 @@ __generic_file_splice_read(struct file *in, loff_t *ppos,
>  			 */
>  			if (!page->mapping) {
>  				unlock_page(page);
> +retry_lookup:
>  				page = find_or_create_page(mapping, index,
>  						mapping_gfp_mask(mapping));
>  
> @@ -439,13 +440,10 @@ __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)
> -					error = 0;
> +					goto retry_lookup;
>  
>  				break;
>  			}
> -- 
> 2.4.3
> 
> 
-- 
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/