kernel BUG at fs/ext4/inode.c:3709! (Re: open bugs found by fuzzing)
diff mbox

Message ID 5788E787.106@oracle.com
State New
Headers show

Commit Message

Vegard Nossum July 15, 2016, 1:39 p.m. UTC
On 07/14/2016 11:10 PM, Vegard Nossum wrote:
> 3. kernel BUG at fs/ext4/inode.c:3709!
> http://139.162.151.198/f/ext4/5bdefda69f39b2f2c56d9b67d5b7d9e2cc8dfd5f

I don't see any evidence of memory corruption here, so this one seems
pretty straightforward: we have an encrypted orphan inode and when we
try to truncate it during the orphan list cleanup it results in a BUG
because we haven't loaded the encryption key for it.

The inode in question has ->i_ino == 16 so I don't think this has
anything to do with special inodes.

Something quick and dirty like this does solve the BUG_ON() for me, but
it looks a lot like papering over an underlying bug:

  	if (ext4_should_journal_data(inode)) {

I'm a bit puzzled that we're actually creating a mapping and trying to
decrypt here in the first place, since if this is an orphan inode that
is being recovered at mount time it means that we know _for sure_ that
there is no existing memory mappings and we're truncating it to 0.

Anyway, adding some Ccs.


Vegard
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Theodore Y. Ts'o July 15, 2016, 5:24 p.m. UTC | #1
On Fri, Jul 15, 2016 at 03:39:19PM +0200, Vegard Nossum wrote:
> 
> I'm a bit puzzled that we're actually creating a mapping and trying to
> decrypt here in the first place, since if this is an orphan inode that
> is being recovered at mount time it means that we know _for sure_ that
> there is no existing memory mappings and we're truncating it to 0.

There are times when we need to make sure i_size is truncated down
(and/or blocks are removed) if we crash in the middle of an operation
that for whatever reason, spans multiple trnsactions.

The simplest such example is truncating down to a non-zero i_size.

If your proposed patch to ext4_block_zero_page_range() helps, then
presumably we're *not* truncating down to zero, but instead truncating
to some non-zero size.  Solving this problem is going to be a bit
tricky.  We could try zeroing the data page at the very begionning of
the truncate operation, but then we could run into the case where data
page write makes it to disk but the truncate oepration was never
committed before a crash.  This would result in a consistent file
system, but the data wouldn't be consistent (that is, it wouldn't be
either the contents before the truncate or after truncate, but
something in between --- so the truncate would no longer be atomic
with respect to crashes).

The other thing I've noticed is I think we're adding some inodes to
the orphan list which shouldn't be necessary if delayed allocation or
data=writeback is enabled.  And when we're deciding whether or not to
add the inode to orphan list we're testing against i_size instead of
i_disksize.  So there are some cases where I think we can avoid adding
inodes to the orphan list in the buffered write case.  And that is a
good thing not just because it avoids the problem with respect to
encrypted inodes, but the orphan list is also a scalability bottleneck
for ext4.

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vegard Nossum July 15, 2016, 5:57 p.m. UTC | #2
On 07/15/2016 07:24 PM, Theodore Ts'o wrote:
> On Fri, Jul 15, 2016 at 03:39:19PM +0200, Vegard Nossum wrote:
>>
>> I'm a bit puzzled that we're actually creating a mapping and trying to
>> decrypt here in the first place, since if this is an orphan inode that
>> is being recovered at mount time it means that we know _for sure_ that
>> there is no existing memory mappings and we're truncating it to 0.
>
> There are times when we need to make sure i_size is truncated down
> (and/or blocks are removed) if we crash in the middle of an operation
> that for whatever reason, spans multiple trnsactions.
>
> The simplest such example is truncating down to a non-zero i_size.
>
> If your proposed patch to ext4_block_zero_page_range() helps, then
> presumably we're *not* truncating down to zero, but instead truncating
> to some non-zero size.

You're right; I just checked, it's truncating it to 4 bytes.

I thought all the inodes on the orphan list were completely unreachable,
but that's obviously not true following your explanations (thanks!) and
another peek at the cleanup function which only does the truncate in the
first place if ->i_nlink is non-zero, I missed that earlier. I guess
this comment confused me:

/* ext4_orphan_cleanup() walks a singly-linked list of inodes (starting at
  * the superblock) which were deleted from all directories, but held 
open by
  * a process at the time of a crash.

But in any case my simple patch is definitely the wrong thing to do.

Thanks,


Vegard
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Y. Ts'o July 15, 2016, 7:49 p.m. UTC | #3
On Fri, Jul 15, 2016 at 07:57:03PM +0200, Vegard Nossum wrote:
> I thought all the inodes on the orphan list were completely unreachable,
> but that's obviously not true following your explanations (thanks!) and
> another peek at the cleanup function which only does the truncate in the
> first place if ->i_nlink is non-zero, I missed that earlier. I guess
> this comment confused me:
> 
> /* ext4_orphan_cleanup() walks a singly-linked list of inodes (starting at
>  * the superblock) which were deleted from all directories, but held open by
>  * a process at the time of a crash.

Yeah, that comment is **badly** (as well over a decade) out-of-date.
Sorry.  :-/

The problem is fixing this is messy.  The problem is if we just set
i_size without zeroing out the bytes between i_size and the end of the
page, we're asking for trouble.  For example, you could think that
it's enough to have ext4_readpage() care of doing the zeroing after
the block is read from disk and then decrypted, but what if the user
does a sparse write beyond i_size?

So either we allow truncate(2) to be non-atomic with respect to
crashes for encrypted files, or alternatively we would have to set a
flag in the inode indicating that the bytes between i_size and the end
of the page must be zeroed before the file can be modified in any way,
or before an attempted O_DIRECT read of the last block, and then when
we read the inode from diks into the inode cache, if the bit is set
and we have the encryption key (which we would need if we want to
modify the file), we could take zeroing the tail end of the file then.

Yulch.   Definitely a bit of a hack.  :-(

					- Ted

P.S. Fortunately, this is a very rare case in practice, so I doubt we
would have hit it in a while, but it's definitely something we need to
fix, one way or another.
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 5a6277d..794b33a 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3735,9 +3735,11 @@  static int __ext4_block_zero_page_range(handle_t 
*handle,
  		if (S_ISREG(inode->i_mode) &&
  		    ext4_encrypted_inode(inode)) {
  			/* We expect the key to be set. */
-			BUG_ON(!fscrypt_has_encryption_key(inode));
-			BUG_ON(blocksize != PAGE_SIZE);
-			WARN_ON_ONCE(fscrypt_decrypt_page(page));
+		        if (list_empty(&EXT4_I(inode)->i_orphan)) {
+				BUG_ON(!fscrypt_has_encryption_key(inode));
+				BUG_ON(blocksize != PAGE_SIZE);
+				WARN_ON_ONCE(fscrypt_decrypt_page(page));
+			}
  		}
  	}