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

Andrew Morton
2004-05-17T06:12:05Z

Linus Torvalds <torvalds@osdl.org> wrote:
>
> Andrew, the obvious culprit would be the memset() in fs/buffer.c
> (block_write_full_page() to be precise):
>
> memset(kaddr + offset, 0, PAGE_CACHE_SIZE - offset);
>
> imagine that the "write()" function updates i_size late - after having
> written out the new contents to the page, and _after_ havign unlocked the
> page, and now we get a writeback at the wrong time, and we decide to clear
> out the end of the page because we think it's past i_size.
>
> Andrew, what do you think?

Interesting. Playing with i_size like that in writepage() _is_ scary. My
immediate reaction is that if this race was real, it's so gross that we
would have spotted it before now in either 2.4 or 2.5->2.6.

Easy test: Steve, could you remove that memset from
block_write_full_page(), see if it changes anything?

It's not very important - it's there because if an application
(incorrectly) writes to mapped data outside EOF we're supposed to drop
their data and write zeroes instead.

> I think this race does exist, since generic_file_aio_write_nolock()
> literally _does_ update i_size only after it has written all the pages, so
> I don't see why a "block_write_full_page()" couldn't come in there between
> and zero them out again at the _old_ i_size boundary.

i_size is updated in generic_commit_write(), on a per-page basis, or I'm
missing something? I sure hope so.

Let's go through the scenarios.

On entry to block_write_full_page(), i_size is in the middle of this page
somewhere. We're worried that i_size can change, and that this will cause
block_write_full_page() to incorrectly zero out the tail of the page.

Well we can stop right there, because the only way someone can get some
more non-zero user data into this page before we memset and write it is by
locking the page beforehand, and block_write_full_page() has the page lock.
(Or they can write stuff into it via mmap, but writing to the page outside
i_size is an application bug).

Other ways in which i_size can change under block_write_full_page()'s feet
are:

- Someone did a truncate.

No problem - the page is about to be invalidated and chopped off the
file anyway.

- Someone did an extending truncate into another page.

OK, i_size will increase but we're still supposed to write zeroes into
the rest of this page outside the previous i_size.

- Someone extended the file into another page with lseek+write or pwrite.

Same argumentation as with extending truncate.

- Someone did an extending truncate to another i_size which lands
*within* this page.

Writing zeroes is still OK: nobody can get into this page to write new
user data anyway - it's locked.

Either all that, or I missed something ;) If Steve can try that test it
would be interesting. Even if removing the memset does make the corruption
go away, this might not be a kernel bug - it could be that the application
is incorrectly relying on mmapped writes outside i_size making it to disk.

As for O_DIRECT: I need to think about that a bit more. We hold i_sem and
have done an fdatasync prior to entering generic_file_aio_write_nolock() so
there should be no dirty pagecache at this stage anyway. The VM may decide
to dirty some pagecache and then block_write_full_page() could come in and
look at i_size and race against generic_file_aio_write_nolock()'s O_DIRECT
i_size_write(). But I doubt if bk is using direct-IO in combination with
MAP_SHARED...