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

Andrew Morton
2004-05-17T07:47:07Z

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.
+ */
+ 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.