linux-kernel - Re: generic_file_splice

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: <20080625141054.GB20851@kernel.dk>
Date:	Wed, 25 Jun 2008 16:10:54 +0200
From:	Jens Axboe <jens.axboe@...cle.com>
To:	Miklos Szeredi <miklos@...redi.hu>
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-mm@...ck.org
Subject: Re: generic_file_splice_read() issues

On Wed, Jun 25 2008, Miklos Szeredi wrote:
> | 	error = 0;
> | 	while (spd.nr_pages < nr_pages) {
> | 		/*
> | 		 * Page could be there, find_get_pages_contig() breaks on
> | 		 * the first hole.
> | 		 */
> | 		page = find_get_page(mapping, index);
> | 		if (!page) {
> | 			/*
> | 			 * page didn't exist, allocate one.
> | 			 */
> | 			page = page_cache_alloc_cold(mapping);
> | 			if (!page)
> 
> error = -ENOMEM?

Looks like it, indeed.

> | 				break;
> | 
> | 			error = add_to_page_cache_lru(page, mapping, index,
> | 						mapping_gfp_mask(mapping));
> | 			if (unlikely(error)) {
> | 				page_cache_release(page);
> | 				if (error == -EEXIST)
> 
> error = 0?  It may not matter, but leaving error as EEXIST is
> confusing at best (and coupled with the above missing ENOMEM could
> result in really weird errors for splice() ;).

Lets move the error assignment inside that loop, ala

-       error = 0;
        while (spd.nr_pages < nr_pages) {
+               error = 0;

But yes, that also looks like a bug. Good spotting!

> | 
> | 			/*
> | 			 * need to read in the page
> | 			 */
> | 			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.
> | 				 */
> | 				if (error == AOP_TRUNCATED_PAGE)
> | 					error = 0;
> 
> This may also cause similar issues as the invalidatation race.  I'd
> think it would be better not to be sloppy here.

Perhaps we can abstract that bit out into a small helper function, tied
in with your previous patch.

-- 
Jens Axboe

--
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/