On Thu, Dec 19, 2013 at 12:43:03AM -0500, Theodore Ts'o wrote:

On Wed, Dec 18, 2013 at 09:12:41PM -0700, Matthew Wilcox wrote:

quoted

Now that I've spent the best part of a day looking at the ext4 code, I
still don't think there's a problem here.  With the way the XIP code is
currently written (calling ext4_get_block with create=1), we won't get an
uninitialised extent in the caller.  Instead, we'll get one that's been
zeroed (the zeroing is part of patch 3/3 and done only for xip files).

If the block was originally allocated via fallocate(2), it will be
marked as uninitialized.  When you call ext4_get_block(), if the block
has been allocated, it will be returned --- and ext4_map_block() as
called by ext4_get_block() does ****not*** clear the uninitialized
flag.  It can't do so because it would be racy; you can only clear the
flag once the data blocks has been written.

As far as patch 3/3, it clears the pages in the page cache, but it
doesn't zap them in the XIP storage device.  But it only does this on
the code path when it allocated a block.  But if the block has already
been previously mapped via fallocate(2), you never hit this section of
code.

Umm.  That sounds like the real bug then.  Any page returned from
get_xip_mem must be initialised, because we may be about to map it
into userspace.

We could have ext4_get_xip_mem() check buffer_unwritten(); if it's set,
zero the blocks and call ext4_convert_unwritten_extents().  Would that
work?

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."