filemap_nopage is broken!!

filemap_nopage is broken!!

[Eric W. Biederman]

While looking at what is needs to happen to get large file support
working on linux-intel, I discovered an interesting twist with
generic_file_mmap.

mmap being the most interesting case to port, because the generic
interface allows for non aligned mappings.  Making it the most
intersting case to handle.

generic_file_mmap allows filesystem block aligned mappings if the
mapping is private.  The way it implements this, after I finally
tracked it is broken.

For private mappings the same filemap_nopage function that is used for
shared mappings is used.  The filemap_nopage function alwasy make sure
it's pages are in the page cache before it uses them.

For a private mapping (not page aligned) this results in a non-aligned
page to be created in the page cache.

Now if the following sequence of actions occure.
a) A page is mapped privately with poor alignment.
b) That part of the file is written again.
c) The page is again mapped privately with poor alignment.

When the page cache page is not scavenged between a and c, the same
data is read, despite the fact it has changed on disk, and in the
aligned page cache page!

That is broken behavior.

Does anyone know where it comes from?

Eric

Re: filemap_nopage is broken!!

[Stephen C. Tweedie]

Hi,

On 22 Apr 1998 15:51:07 -0500, ebiederm+eric@npwt.net (Eric
W. Biederman) said:

> Now if the following sequence of actions occure.
> a) A page is mapped privately with poor alignment.
> b) That part of the file is written again.
> c) The page is again mapped privately with poor alignment.

> When the page cache page is not scavenged between a and c, the same
> data is read, despite the fact it has changed on disk, and in the
> aligned page cache page!

> That is broken behavior.

I don't think this is necessarily a problem.  The kernel simply does not
guarantee full correspondance semantics between filesystem updates and
the page cache for non-aligned pages, but then again, it is not required
to --- it is not even required to support such mmaps, so I can live with
an undefined behaviour in this case!

--Stephen

Re: filemap_nopage is broken!!

[Eric W. Biederman]

>>>>> "ST" == Stephen C Tweedie <sct@dcs.ed.ac.uk> writes:

ST> I don't think this is necessarily a problem.  The kernel simply does not
ST> guarantee full correspondance semantics between filesystem updates and
ST> the page cache for non-aligned pages, but then again, it is not required
ST> to --- it is not even required to support such mmaps, so I can live with
ST> an undefined behaviour in this case!

Ah, but suppose we have a mythological a.out programmer.
This programmer could run a program, doesn't like the result, compiles
a new version which overwrites the old, and attempts to execute the
new program.  And executes the old!

There may be a lock in there that I haven't spotted, and likely there
will be a truncation when the file is overwritten which would flush
the page cache but it is possible there isn't.

As the kernel internally uses these mappings for a.out executables
this is an undefined case which propogates.  It's undefined which
a.out program executes :(  That part is much harder to live with.

I guess what is I find most objectionable is 
a) There is no big fat warning anywhere.
b) The current implementation will pass simple tests so it will look
   like it works, and then fail at strange weird unpredictable times.

I doubt it will be anything like a show stopper for 2.2 but if this
code get's touched it should be fixed to do something consistent.  

Eric

Re: filemap_nopage is broken!!

[Stephen C. Tweedie]

Hi,

On 23 Apr 1998 19:51:16 -0500, ebiederm+eric@npwt.net (Eric
W. Biederman) said:

>>>>>> "ST" == Stephen C Tweedie <sct@dcs.ed.ac.uk> writes:
ST> I don't think this is necessarily a problem.  The kernel simply does not
ST> guarantee full correspondance semantics between filesystem updates and
ST> the page cache for non-aligned pages, but then again, it is not required
ST> to --- it is not even required to support such mmaps, so I can live with
ST> an undefined behaviour in this case!

> Ah, but suppose we have a mythological a.out programmer.
> This programmer could run a program, doesn't like the result, compiles
> a new version which overwrites the old, and attempts to execute the
> new program.  And executes the old!

> There may be a lock in there that I haven't spotted, and likely there
> will be a truncation when the file is overwritten which would flush
> the page cache but it is possible there isn't.

There is.  truncate_inode_pages() will invalidate all of the mappings
when a file is either truncated or deleted.  Any overwrite of the file
will do the right thing.

> I doubt it will be anything like a show stopper for 2.2 but if this
> code get's touched it should be fixed to do something consistent.  

I don't think there's any problem.

--Stephen