[PATCH/RFC] Is it OK for 'read' to return nuls for a file that never had nuls in it?

[PATCH/RFC] Is it OK for 'read' to return nuls for a file that never had nuls in it?

[Neil Brown]

[resending with correct To address - please reply to this one]

It appears that there is a race when reading from a file that is
concurrently being truncated.  It is possible to read a number of
bytes that matches the size of the file before the truncate, but the
actual bytes are all nuls - values that had never been in the file.

Below is a simple C program to demonstrate this, and a patch that
might fix it (initial testing is positive, but it might just make the
window smaller).
To trial the program run two instances, naming the same file as the
only argument.  Every '!' indicates a read that found a nul.  I get
one every few minutes.
e.g.  cc -o race race.c ; ./race /tmp/testfile & ./race /tmp/tracefile

My exploration suggests the problem is in do_generic_mapping_read in
mm/filemap.c. 
This code:
   gets the size of the file
   triggers readahead
   gets the appropriate page
   If the page is up-to-date, return data.

If a truncate happens just before readahead is triggered, then
the size will be the pre-truncate size of the file, while the page
could have been read by the readahead and so will be up-to-date and
full of nuls.

Note that if do_generic_mapping_read calls readpage explicitly, it
samples the value of inode->i_size again after the read.  However if
the readpage is called by the readahead code, i_size is not
re-sampled.

I am not 100% confident of every aspect of this explanation (I haven't
traced all the way through the read-ahead code) but it seems to fit
the available data including the fact that if I disable read-ahead
(blockdev --setra 0) then the apparent problem goes away.

The patch below moves the code for re-sampling i_size from after the
readpage call to before the "actor" call.

Questions:
  - Is this a problem, and should it be fixed (I think "yes").
  - Is the patch appropriate, and does it have no negative
    consequences?.
    (Obviously some comments should be tidied up to reflect the new
    reality).

Thanks,
NeilBrown

------------------------------------------------------------
#include <unistd.h>
#include <stdlib.h>
#include <fcntl.h>
#include <stdio.h>

int main(int argc, char *argv[])
{
	char buf1[4096];
	char buf2[4096];
	int i;

	for (i=0; i<4096; i++) buf1[i] = 'x';

	while(1) {
		int fd = open(argv[1], O_WRONLY|O_CREAT|O_TRUNC, 0600);
		int n;
		if (fd < 0) {
			perror("open-write");
			exit(1);
		}
		if (write(fd, buf1, 4096) != 4096) {
			perror("write1");
			exit(1);
		}
		close(fd);
		fd = open(argv[1], O_RDONLY, 0600);
		if (fd < 0) {
			perror("open-read");
			exit(1);
		}
		n = read(fd, buf2, 4096);
		if (n == 0) {
			// printf(".");
			fflush(stdout);
			close(fd);
			continue;
		}
		if (n != 4096) {
			perror("read1");
			exit(1);
		}
		if (buf2[0] == 0) {
			printf("!");
			fflush(stdout);
		}
		close(fd);
	}
}

---------------------------------------------------------
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./mm/filemap.c |   35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff .prev/mm/filemap.c ./mm/filemap.c
--- .prev/mm/filemap.c	2007-05-29 09:41:06.000000000 +1000
+++ ./mm/filemap.c	2007-05-29 12:06:03.000000000 +1000
@@ -930,6 +930,24 @@ find_page:
 			goto page_not_up_to_date;
 page_ok:
 
+		isize = i_size_read(inode);
+		end_index = (isize - 1) >> PAGE_CACHE_SHIFT;
+		if (unlikely(!isize || index > end_index)) {
+			page_cache_release(page);
+			goto out;
+		}
+
+		/* nr is the maximum number of bytes to copy from this page */
+		nr = PAGE_CACHE_SIZE;
+		if (index == end_index) {
+			nr = ((isize - 1) & ~PAGE_CACHE_MASK) + 1;
+			if (nr <= offset) {
+				page_cache_release(page);
+				goto out;
+			}
+		}
+		nr = nr - offset;
+
 		/* If users can be writing to this page using arbitrary
 		 * virtual addresses, take care about potential aliasing
 		 * before reading the page on the kernel side.
@@ -1023,23 +1041,6 @@ readpage:
 		 * part of the page is not copied back to userspace (unless
 		 * another truncate extends the file - this is desired though).
 		 */
-		isize = i_size_read(inode);
-		end_index = (isize - 1) >> PAGE_CACHE_SHIFT;
-		if (unlikely(!isize || index > end_index)) {
-			page_cache_release(page);
-			goto out;
-		}
-
-		/* nr is the maximum number of bytes to copy from this page */
-		nr = PAGE_CACHE_SIZE;
-		if (index == end_index) {
-			nr = ((isize - 1) & ~PAGE_CACHE_MASK) + 1;
-			if (nr <= offset) {
-				page_cache_release(page);
-				goto out;
-			}
-		}
-		nr = nr - offset;
 		goto page_ok;
 
 readpage_error:

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH/RFC] Is it OK for 'read' to return nuls for a file that never had nuls in it?

[Nick Piggin]

[-- Attachment #1: Type: text/plain, Size: 2346 bytes --]

Neil Brown wrote:
> [resending with correct To address - please reply to this one]
> 
> It appears that there is a race when reading from a file that is
> concurrently being truncated.  It is possible to read a number of
> bytes that matches the size of the file before the truncate, but the
> actual bytes are all nuls - values that had never been in the file.
> 
> Below is a simple C program to demonstrate this, and a patch that
> might fix it (initial testing is positive, but it might just make the
> window smaller).
> To trial the program run two instances, naming the same file as the
> only argument.  Every '!' indicates a read that found a nul.  I get
> one every few minutes.
> e.g.  cc -o race race.c ; ./race /tmp/testfile & ./race /tmp/tracefile
> 
> My exploration suggests the problem is in do_generic_mapping_read in
> mm/filemap.c. 
> This code:
>    gets the size of the file
>    triggers readahead
>    gets the appropriate page
>    If the page is up-to-date, return data.
> 
> If a truncate happens just before readahead is triggered, then
> the size will be the pre-truncate size of the file, while the page
> could have been read by the readahead and so will be up-to-date and
> full of nuls.
> 
> Note that if do_generic_mapping_read calls readpage explicitly, it
> samples the value of inode->i_size again after the read.  However if
> the readpage is called by the readahead code, i_size is not
> re-sampled.
> 
> I am not 100% confident of every aspect of this explanation (I haven't
> traced all the way through the read-ahead code) but it seems to fit
> the available data including the fact that if I disable read-ahead
> (blockdev --setra 0) then the apparent problem goes away.
> 
> The patch below moves the code for re-sampling i_size from after the
> readpage call to before the "actor" call.
> 
> Questions:
>   - Is this a problem, and should it be fixed (I think "yes").

I think you are right.

>   - Is the patch appropriate, and does it have no negative
>     consequences?.
>     (Obviously some comments should be tidied up to reflect the new
>     reality).

Would it be better (and closer to following the existing logic) if
we sampled i_size before testing each page for uptodateness? It might
also cost a little less in the fastpath case of finding an uptodate
page.

-- 
SUSE Labs, Novell Inc.

[-- Attachment #2: mm-ra-zerofill-fix.patch --]
[-- Type: text/plain, Size: 1235 bytes --]

Index: linux-2.6/mm/filemap.c
===================================================================
--- linux-2.6.orig/mm/filemap.c	2007-05-29 16:34:38.000000000 +1000
+++ linux-2.6/mm/filemap.c	2007-05-29 16:35:42.000000000 +1000
@@ -863,13 +863,11 @@ void do_generic_mapping_read(struct addr
 {
 	struct inode *inode = mapping->host;
 	unsigned long index;
-	unsigned long end_index;
 	unsigned long offset;
 	unsigned long last_index;
 	unsigned long next_index;
 	unsigned long prev_index;
 	unsigned int prev_offset;
-	loff_t isize;
 	struct page *cached_page;
 	int error;
 	struct file_ra_state ra = *_ra;
@@ -882,15 +880,17 @@ void do_generic_mapping_read(struct addr
 	last_index = (*ppos + desc->count + PAGE_CACHE_SIZE-1) >> PAGE_CACHE_SHIFT;
 	offset = *ppos & ~PAGE_CACHE_MASK;
 
-	isize = i_size_read(inode);
-	if (!isize)
-		goto out;
-
-	end_index = (isize - 1) >> PAGE_CACHE_SHIFT;
 	for (;;) {
+		loff_t isize;
 		struct page *page;
+		unsigned long end_index;
 		unsigned long nr, ret;
 
+		isize = i_size_read(inode);
+		if (!isize)
+			goto out;
+		end_index = (isize - 1) >> PAGE_CACHE_SHIFT;
+
 		/* nr is the maximum number of bytes to copy from this page */
 		nr = PAGE_CACHE_SIZE;
 		if (index >= end_index) {

Re: [PATCH/RFC] Is it OK for 'read' to return nuls for a file that never had nuls in it?

[Neil Brown]

On Tuesday May 29, nickpiggin@yahoo.com.au wrote:
> > 
> > Questions:
> >   - Is this a problem, and should it be fixed (I think "yes").
> 
> I think you are right.

That's encouraging - thanks.

> 
> >   - Is the patch appropriate, and does it have no negative
> >     consequences?.
> >     (Obviously some comments should be tidied up to reflect the new
> >     reality).
> 
> Would it be better (and closer to following the existing logic) if
> we sampled i_size before testing each page for uptodateness? It might
> also cost a little less in the fastpath case of finding an uptodate
> page.

Uhm. no.

It doesn't address the issue which is that we need to check i_size
*after* we have got hold of the page and believe it to be uptodate.

And just to check, I race the test with your patch and I get a nice
little row of "!!!!!" - it is finding zeros still.

But we could possibly remove the early checks on end_index, that
makes sense (I've also moved the comment).

Thanks,
NeilBrown


Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./mm/filemap.c |   72 ++++++++++++++++++++++-----------------------------------
 1 file changed, 28 insertions(+), 44 deletions(-)

diff .prev/mm/filemap.c ./mm/filemap.c
--- .prev/mm/filemap.c	2007-05-29 16:45:26.000000000 +1000
+++ ./mm/filemap.c	2007-05-29 16:58:23.000000000 +1000
@@ -875,13 +875,11 @@ void do_generic_mapping_read(struct addr
 {
 	struct inode *inode = mapping->host;
 	unsigned long index;
-	unsigned long end_index;
 	unsigned long offset;
 	unsigned long last_index;
 	unsigned long next_index;
 	unsigned long prev_index;
 	unsigned int prev_offset;
-	loff_t isize;
 	struct page *cached_page;
 	int error;
 	struct file_ra_state ra = *_ra;
@@ -894,27 +892,12 @@ void do_generic_mapping_read(struct addr
 	last_index = (*ppos + desc->count + PAGE_CACHE_SIZE-1) >> PAGE_CACHE_SHIFT;
 	offset = *ppos & ~PAGE_CACHE_MASK;
 
-	isize = i_size_read(inode);
-	if (!isize)
-		goto out;
-
-	end_index = (isize - 1) >> PAGE_CACHE_SHIFT;
 	for (;;) {
 		struct page *page;
+		unsigned long end_index;
+		loff_t isize;
 		unsigned long nr, ret;
 
-		/* nr is the maximum number of bytes to copy from this page */
-		nr = PAGE_CACHE_SIZE;
-		if (index >= end_index) {
-			if (index > end_index)
-				goto out;
-			nr = ((isize - 1) & ~PAGE_CACHE_MASK) + 1;
-			if (nr <= offset) {
-				goto out;
-			}
-		}
-		nr = nr - offset;
-
 		cond_resched();
 		if (index == next_index)
 			next_index = page_cache_readahead(mapping, &ra, filp,
@@ -929,6 +912,32 @@ find_page:
 		if (!PageUptodate(page))
 			goto page_not_up_to_date;
 page_ok:
+		/*
+		 * i_size must be checked after we know the page is Uptodate.
+		 *
+		 * Checking i_size after the check allows us to calculate
+		 * the correct value for "nr", which means the zero-filled
+		 * part of the page is not copied back to userspace (unless
+		 * another truncate extends the file - this is desired though).
+		 */
+
+		isize = i_size_read(inode);
+		end_index = (isize - 1) >> PAGE_CACHE_SHIFT;
+		if (unlikely(!isize || index > end_index)) {
+			page_cache_release(page);
+			goto out;
+		}
+
+		/* nr is the maximum number of bytes to copy from this page */
+		nr = PAGE_CACHE_SIZE;
+		if (index == end_index) {
+			nr = ((isize - 1) & ~PAGE_CACHE_MASK) + 1;
+			if (nr <= offset) {
+				page_cache_release(page);
+				goto out;
+			}
+		}
+		nr = nr - offset;
 
 		/* If users can be writing to this page using arbitrary
 		 * virtual addresses, take care about potential aliasing
@@ -1015,31 +1024,6 @@ readpage:
 			unlock_page(page);
 		}
 
-		/*
-		 * i_size must be checked after we have done ->readpage.
-		 *
-		 * Checking i_size after the readpage allows us to calculate
-		 * the correct value for "nr", which means the zero-filled
-		 * part of the page is not copied back to userspace (unless
-		 * another truncate extends the file - this is desired though).
-		 */
-		isize = i_size_read(inode);
-		end_index = (isize - 1) >> PAGE_CACHE_SHIFT;
-		if (unlikely(!isize || index > end_index)) {
-			page_cache_release(page);
-			goto out;
-		}
-
-		/* nr is the maximum number of bytes to copy from this page */
-		nr = PAGE_CACHE_SIZE;
-		if (index == end_index) {
-			nr = ((isize - 1) & ~PAGE_CACHE_MASK) + 1;
-			if (nr <= offset) {
-				page_cache_release(page);
-				goto out;
-			}
-		}
-		nr = nr - offset;
 		goto page_ok;
 
 readpage_error:

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH/RFC] Is it OK for 'read' to return nuls for a file that never had nuls in it?

[Nick Piggin]

Neil Brown wrote:
> On Tuesday May 29, nickpiggin@yahoo.com.au wrote:
> 
>>>Questions:
>>>  - Is this a problem, and should it be fixed (I think "yes").
>>
>>I think you are right.
> 
> 
> That's encouraging - thanks.
> 
> 
>>>  - Is the patch appropriate, and does it have no negative
>>>    consequences?.
>>>    (Obviously some comments should be tidied up to reflect the new
>>>    reality).
>>
>>Would it be better (and closer to following the existing logic) if
>>we sampled i_size before testing each page for uptodateness? It might
>>also cost a little less in the fastpath case of finding an uptodate
>>page.
> 
> 
> Uhm. no.
> 
> It doesn't address the issue which is that we need to check i_size
> *after* we have got hold of the page and believe it to be uptodate.

Hmm OK, I thought the logic worked for a single page but was broken for
subsequent patches... but we can readahead the first page before checking
it is uptodate, can't we? (or even if we didn't, someone else might
concurrently).

But then I think a problem remains after your patch that if the page is
partially truncated after you test that it is uptodate and resample i_size,
then the page tail can be zero filled and then you'll again get back a
nul tail from read(2), don't we? We could probably fix this beautifully by
doing a lock_page over do_generic_mapping_read... ha ha, that would be
popular.

For now I think your patch probably eliminates some classes of the bug
completely and remainder are a small race-window rather than a straight-line
bug, so it is probably the best way to go for now. I'd say
Acked-by: Nick Piggin <npiggin@suse.de>. Ram Pai I believe also worked on
similar issues with me, so I'll cc him.

Longer term, I have a few other issues with the fs layer returning non
uptodate pages to do_generic_mapping_read which could also require wider
scale rework...

> 
> And just to check, I race the test with your patch and I get a nice
> little row of "!!!!!" - it is finding zeros still.
> 
> But we could possibly remove the early checks on end_index, that
> makes sense (I've also moved the comment).
> 
> Thanks,
> NeilBrown
> 
> 
> Signed-off-by: Neil Brown <neilb@suse.de>
> 
> ### Diffstat output
>  ./mm/filemap.c |   72 ++++++++++++++++++++++-----------------------------------
>  1 file changed, 28 insertions(+), 44 deletions(-)
> 
> diff .prev/mm/filemap.c ./mm/filemap.c
> --- .prev/mm/filemap.c	2007-05-29 16:45:26.000000000 +1000
> +++ ./mm/filemap.c	2007-05-29 16:58:23.000000000 +1000
> @@ -875,13 +875,11 @@ void do_generic_mapping_read(struct addr
>  {
>  	struct inode *inode = mapping->host;
>  	unsigned long index;
> -	unsigned long end_index;
>  	unsigned long offset;
>  	unsigned long last_index;
>  	unsigned long next_index;
>  	unsigned long prev_index;
>  	unsigned int prev_offset;
> -	loff_t isize;
>  	struct page *cached_page;
>  	int error;
>  	struct file_ra_state ra = *_ra;
> @@ -894,27 +892,12 @@ void do_generic_mapping_read(struct addr
>  	last_index = (*ppos + desc->count + PAGE_CACHE_SIZE-1) >> PAGE_CACHE_SHIFT;
>  	offset = *ppos & ~PAGE_CACHE_MASK;
>  
> -	isize = i_size_read(inode);
> -	if (!isize)
> -		goto out;
> -
> -	end_index = (isize - 1) >> PAGE_CACHE_SHIFT;
>  	for (;;) {
>  		struct page *page;
> +		unsigned long end_index;
> +		loff_t isize;
>  		unsigned long nr, ret;
>  
> -		/* nr is the maximum number of bytes to copy from this page */
> -		nr = PAGE_CACHE_SIZE;
> -		if (index >= end_index) {
> -			if (index > end_index)
> -				goto out;
> -			nr = ((isize - 1) & ~PAGE_CACHE_MASK) + 1;
> -			if (nr <= offset) {
> -				goto out;
> -			}
> -		}
> -		nr = nr - offset;
> -
>  		cond_resched();
>  		if (index == next_index)
>  			next_index = page_cache_readahead(mapping, &ra, filp,
> @@ -929,6 +912,32 @@ find_page:
>  		if (!PageUptodate(page))
>  			goto page_not_up_to_date;
>  page_ok:
> +		/*
> +		 * i_size must be checked after we know the page is Uptodate.
> +		 *
> +		 * Checking i_size after the check allows us to calculate
> +		 * the correct value for "nr", which means the zero-filled
> +		 * part of the page is not copied back to userspace (unless
> +		 * another truncate extends the file - this is desired though).
> +		 */
> +
> +		isize = i_size_read(inode);
> +		end_index = (isize - 1) >> PAGE_CACHE_SHIFT;
> +		if (unlikely(!isize || index > end_index)) {
> +			page_cache_release(page);
> +			goto out;
> +		}
> +
> +		/* nr is the maximum number of bytes to copy from this page */
> +		nr = PAGE_CACHE_SIZE;
> +		if (index == end_index) {
> +			nr = ((isize - 1) & ~PAGE_CACHE_MASK) + 1;
> +			if (nr <= offset) {
> +				page_cache_release(page);
> +				goto out;
> +			}
> +		}
> +		nr = nr - offset;
>  
>  		/* If users can be writing to this page using arbitrary
>  		 * virtual addresses, take care about potential aliasing
> @@ -1015,31 +1024,6 @@ readpage:
>  			unlock_page(page);
>  		}
>  
> -		/*
> -		 * i_size must be checked after we have done ->readpage.
> -		 *
> -		 * Checking i_size after the readpage allows us to calculate
> -		 * the correct value for "nr", which means the zero-filled
> -		 * part of the page is not copied back to userspace (unless
> -		 * another truncate extends the file - this is desired though).
> -		 */
> -		isize = i_size_read(inode);
> -		end_index = (isize - 1) >> PAGE_CACHE_SHIFT;
> -		if (unlikely(!isize || index > end_index)) {
> -			page_cache_release(page);
> -			goto out;
> -		}
> -
> -		/* nr is the maximum number of bytes to copy from this page */
> -		nr = PAGE_CACHE_SIZE;
> -		if (index == end_index) {
> -			nr = ((isize - 1) & ~PAGE_CACHE_MASK) + 1;
> -			if (nr <= offset) {
> -				page_cache_release(page);
> -				goto out;
> -			}
> -		}
> -		nr = nr - offset;
>  		goto page_ok;
>  
>  readpage_error:
> 


-- 
SUSE Labs, Novell Inc.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH/RFC] Is it OK for 'read' to return nuls for a file that never had nuls in it?

[Neil Brown]

On Tuesday May 29, nickpiggin@yahoo.com.au wrote:
> 
> But then I think a problem remains after your patch that if the page is
> partially truncated after you test that it is uptodate and resample i_size,
> then the page tail can be zero filled and then you'll again get back a
> nul tail from read(2), don't we? We could probably fix this beautifully by
> doing a lock_page over do_generic_mapping_read... ha ha, that would be
> popular.
> 
> For now I think your patch probably eliminates some classes of the bug
> completely and remainder are a small race-window rather than a straight-line
> bug, so it is probably the best way to go for now. I'd say
> Acked-by: Nick Piggin <npiggin@suse.de>. Ram Pai I believe also worked on
> similar issues with me, so I'll cc him.

Yes, the race with truncate_partial_page had occurred to me too.  It
can zero-out part of a page at any time with-respect-to
do_generic_mapping_read.  Apart from locking the page (which is
unlikely to go down well) the only solution I can think of is to check
the size again afterwards and fix things up if we over-shot the new
end-of-file.  We would at-least need to fix up 'ret' and
desc->written, and maybe desc->count and desc->arg.buf as well -
sounds messy.  Best to just leave it for now?

As an aside, what do you suppose should happen in the face of a race
between readv and extension of the file.
To be more specific, suppose we do a readv passing an iovec holding 2
1K buffers.  Suppose further that at this point in time we are 512
bytes from the end of the file.
If do_readv_writev takes the do_sync_readv_writev branch and calls
generic_file_aio_read, it will simply call do_generic_read_file for
each of the two buffers.
The first gets 512 bytes.  Then someone extends the file before the
second call which - for example - gets another 512 bytes.
So readv returns 1024, but the bytes aren't all in the first buffer.
They are half in the first buffer and half in the second.

So do we need this patch?  (It is my fourth attempt at getting the
logic right, but it now looks similar to the logic in
do_loop_readv_writev, so that is encouraging).

NeilBrown

Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./mm/filemap.c |    2 ++
 1 file changed, 2 insertions(+)

diff .prev/mm/filemap.c ./mm/filemap.c
--- .prev/mm/filemap.c	2007-05-29 16:45:26.000000000 +1000
+++ ./mm/filemap.c	2007-05-31 16:49:45.000000000 +1000
@@ -1227,6 +1227,8 @@ generic_file_aio_read(struct kiocb *iocb
 				retval = retval ?: desc.error;
 				break;
 			}
+			if (desc.count > 0)
+				break;
 		}
 	}
 out:

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>