block_page_mkwrite() bug or feature?

block_page_mkwrite() bug or feature?

[Anton Altaparmakov]

Hi,

Perhaps I am missing something (in which case I would appreciate a hit with a clue bat!) but it looks to me like there is a bug in fs/buffer.c::block_page_mkwrite().

Consider the case where "size = i_size_read()" is X and page_offset() is also X, i.e. the page being made dirty starts at the end of the file.

In this case the first check passes because page_offset == size rather than being > size - This looks wrong to me - surely the page is entirely outside the file size "size" even when its first byte is at "size" offset as "size" is the first byte outside the file:

	if ((page->mapping != inode->i_mapping) ||
	    (page_offset(page) > size)) { 

After that is passed the next code happens:

	/* page is wholly or partially inside EOF */
        if (((page->index + 1) << PAGE_CACHE_SHIFT) > size)
                end = size & ~PAGE_CACHE_MASK;
        else
                end = PAGE_CACHE_SIZE;

We now do have a "true" condition in the if clause and we also know that "size" is page aligned thus "end = size & ~PAGE_CACHE_MASK" evaluates as "end = 0;".

Thus we then get __block_write_begin() being called with "end" equal 0:

	ret = __block_write_begin(page, 0, end, get_block);
        if (!ret)
                ret = block_commit_write(page, 0, end);

So we get __block_write_begin an then block_commit_write with both pos and len being zero.

Then the page is marked dirty and we return it.

Unless I am correct that this is a bug, what is the reasoning for doing this instead of simply bailing out on the page straight away at the top of the function by changing the test to page_offset(page) >= size?

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge
J.J. Thomson Avenue, Cambridge, CB3 0RB, UK

Re: block_page_mkwrite() bug or feature?

[Jan Kara]

  Hello,

On Fri 02-05-14 22:52:43, Anton Altaparmakov wrote:
> Perhaps I am missing something (in which case I would appreciate a hit
> with a clue bat!) but it looks to me like there is a bug in
> fs/buffer.c::block_page_mkwrite().
> 
> Consider the case where "size = i_size_read()" is X and page_offset() is
> also X, i.e. the page being made dirty starts at the end of the file.
> 
> In this case the first check passes because page_offset == size rather
> than being > size - This looks wrong to me - surely the page is entirely
> outside the file size "size" even when its first byte is at "size" offset
> as "size" is the first byte outside the file:
> 
> 	if ((page->mapping != inode->i_mapping) ||
> 	    (page_offset(page) > size)) { 
  Yup, that looks like a bug, although a pretty harmless one since such a
call can only happen when truncating mmaped file before we manage to
invalidate the page beyond EOF.

								Honza

> 
> After that is passed the next code happens:
> 
> 	/* page is wholly or partially inside EOF */
>         if (((page->index + 1) << PAGE_CACHE_SHIFT) > size)
>                 end = size & ~PAGE_CACHE_MASK;
>         else
>                 end = PAGE_CACHE_SIZE;
> 
> We now do have a "true" condition in the if clause and we also know that "size" is page aligned thus "end = size & ~PAGE_CACHE_MASK" evaluates as "end = 0;".
> 
> Thus we then get __block_write_begin() being called with "end" equal 0:
> 
> 	ret = __block_write_begin(page, 0, end, get_block);
>         if (!ret)
>                 ret = block_commit_write(page, 0, end);
> 
> So we get __block_write_begin an then block_commit_write with both pos and len being zero.
> 
> Then the page is marked dirty and we return it.
> 
> Unless I am correct that this is a bug, what is the reasoning for doing this instead of simply bailing out on the page straight away at the top of the function by changing the test to page_offset(page) >= size?
> 
> Best regards,
> 
> 	Anton
> -- 
> Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
> Unix Support, Computing Service, University of Cambridge
> J.J. Thomson Avenue, Cambridge, CB3 0RB, UK
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR