Re: 1352 NUL bytes at the end of a page? (was Re: Assertion `s && s->tree' failed: The saga continues.)

[Vladimir Saveliev <vs@namesys.com>]

Hello

On Mon, 2004-05-17 at 11:46, Andrew Morton wrote:
> Andrew Morton <akpm@osdl.org> wrote:
> >
> >  If an application does mmap(MAP_SHARED) of, say, a 2048 byte file and then
> >  extends it:
> > 
> >  	p = mmap(..., fd, ...);
> >  	ftructate(fd, 4096);
> >  	p[3000] = 1;
> > 
> >  A racing block_write_full_page() could fail to notice the extended i_size
> >  and would decide to zap those 2048 bytes anyway.
> 
> This should plug it.
> 
> diff -puN mm/memory.c~ftruncate-vs-block_write_full_page mm/memory.c
> --- 25/mm/memory.c~ftruncate-vs-block_write_full_page	2004-05-17 00:33:07.060231368 -0700
> +++ 25-akpm/mm/memory.c	2004-05-17 00:41:00.924193096 -0700
> @@ -1208,6 +1208,8 @@ int vmtruncate(struct inode * inode, lof
>  {
>  	struct address_space *mapping = inode->i_mapping;
>  	unsigned long limit;
> +	loff_t i_size;
> +	struct page *page;
>  
>  	if (inode->i_size < offset)
>  		goto do_expand;
> @@ -1222,8 +1224,22 @@ do_expand:
>  		goto out_sig;
>  	if (offset > inode->i_sb->s_maxbytes)
>  		goto out;
> -	i_size_write(inode, offset);
>  
> +	/*
> +	 * If there is a pagecache page at the current i_size we need to lock
> +	 * it while modifying i_size to synchronise against
> +	 * block_write_full_page()'s sampling of i_size.  Otherwise
> +	 * block_write_full_page may decide to memset part of this page after
> +	 * the application extended the file size.
> +	 */

Don't down-ings i_sem in do_truncate and in generic_file_write take care
of this kind of race?

> +	i_size = inode->i_size;	/* don't need i_size_read() due to i_sem */
> +	page = NULL;
> +	if (i_size & (PAGE_CACHE_SIZE - 1))
> +		page = find_lock_page(inode->i_mapping,
> +				i_size >> PAGE_CACHE_SHIFT);
> +	i_size_write(inode, offset);
> +	if (page)
> +		unlock_page(page);
>  out_truncate:
>  	if (inode->i_op && inode->i_op->truncate)
>  		inode->i_op->truncate(inode);
> 
> _
> 
> The same could happen with a pwrite() in place of ftruncate:
> 
> 	fd = open("2048-byte-file");
> 	p = mmap(..., MAP_SHARED, fd, ...);
> 	pwrite(fd, buf, 1, 4096);
> 	p[3000] = 1;
> 
> But I doubt that bk does extending writes() against a file which is
> concurrently being modified via MAP_SHARED.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>