| From 9d506594069355d1fb2de3f9104667312ff08ed3 Mon Sep 17 00:00:00 2001 |
| From: Lukas Czerner <lczerner@redhat.com> |
| Date: Thu, 14 May 2015 18:55:18 -0400 |
| Subject: ext4: fix NULL pointer dereference when journal restart fails |
| |
| From: Lukas Czerner <lczerner@redhat.com> |
| |
| commit 9d506594069355d1fb2de3f9104667312ff08ed3 upstream. |
| |
| Currently when journal restart fails, we'll have the h_transaction of |
| the handle set to NULL to indicate that the handle has been effectively |
| aborted. We handle this situation quietly in the jbd2_journal_stop() and just |
| free the handle and exit because everything else has been done before we |
| attempted (and failed) to restart the journal. |
| |
| Unfortunately there are a number of problems with that approach |
| introduced with commit |
| |
| 41a5b913197c "jbd2: invalidate handle if jbd2_journal_restart() |
| fails" |
| |
| First of all in ext4 jbd2_journal_stop() will be called through |
| __ext4_journal_stop() where we would try to get a hold of the superblock |
| by dereferencing h_transaction which in this case would lead to NULL |
| pointer dereference and crash. |
| |
| In addition we're going to free the handle regardless of the refcount |
| which is bad as well, because others up the call chain will still |
| reference the handle so we might potentially reference already freed |
| memory. |
| |
| Moreover it's expected that we'll get aborted handle as well as detached |
| handle in some of the journalling function as the error propagates up |
| the stack, so it's unnecessary to call WARN_ON every time we get |
| detached handle. |
| |
| And finally we might leak some memory by forgetting to free reserved |
| handle in jbd2_journal_stop() in the case where handle was detached from |
| the transaction (h_transaction is NULL). |
| |
| Fix the NULL pointer dereference in __ext4_journal_stop() by just |
| calling jbd2_journal_stop() quietly as suggested by Jan Kara. Also fix |
| the potential memory leak in jbd2_journal_stop() and use proper |
| handle refcounting before we attempt to free it to avoid use-after-free |
| issues. |
| |
| And finally remove all WARN_ON(!transaction) from the code so that we do |
| not get random traces when something goes wrong because when journal |
| restart fails we will get to some of those functions. |
| |
| Signed-off-by: Lukas Czerner <lczerner@redhat.com> |
| Signed-off-by: Theodore Ts'o <tytso@mit.edu> |
| Reviewed-by: Jan Kara <jack@suse.cz> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| |
| --- |
| fs/ext4/ext4_jbd2.c | 6 ++++++ |
| fs/jbd2/transaction.c | 25 ++++++++++++++++--------- |
| 2 files changed, 22 insertions(+), 9 deletions(-) |
| |
| --- a/fs/ext4/ext4_jbd2.c |
| +++ b/fs/ext4/ext4_jbd2.c |
| @@ -87,6 +87,12 @@ int __ext4_journal_stop(const char *wher |
| ext4_put_nojournal(handle); |
| return 0; |
| } |
| + |
| + if (!handle->h_transaction) { |
| + err = jbd2_journal_stop(handle); |
| + return handle->h_err ? handle->h_err : err; |
| + } |
| + |
| sb = handle->h_transaction->t_journal->j_private; |
| err = handle->h_err; |
| rc = jbd2_journal_stop(handle); |
| --- a/fs/jbd2/transaction.c |
| +++ b/fs/jbd2/transaction.c |
| @@ -551,7 +551,6 @@ int jbd2_journal_extend(handle_t *handle |
| int result; |
| int wanted; |
| |
| - WARN_ON(!transaction); |
| if (is_handle_aborted(handle)) |
| return -EROFS; |
| journal = transaction->t_journal; |
| @@ -627,7 +626,6 @@ int jbd2__journal_restart(handle_t *hand |
| tid_t tid; |
| int need_to_start, ret; |
| |
| - WARN_ON(!transaction); |
| /* If we've had an abort of any type, don't even think about |
| * actually doing the restart! */ |
| if (is_handle_aborted(handle)) |
| @@ -791,7 +789,6 @@ do_get_write_access(handle_t *handle, st |
| int need_copy = 0; |
| unsigned long start_lock, time_lock; |
| |
| - WARN_ON(!transaction); |
| if (is_handle_aborted(handle)) |
| return -EROFS; |
| journal = transaction->t_journal; |
| @@ -1057,7 +1054,6 @@ int jbd2_journal_get_create_access(handl |
| int err; |
| |
| jbd_debug(5, "journal_head %p\n", jh); |
| - WARN_ON(!transaction); |
| err = -EROFS; |
| if (is_handle_aborted(handle)) |
| goto out; |
| @@ -1271,7 +1267,6 @@ int jbd2_journal_dirty_metadata(handle_t |
| struct journal_head *jh; |
| int ret = 0; |
| |
| - WARN_ON(!transaction); |
| if (is_handle_aborted(handle)) |
| return -EROFS; |
| journal = transaction->t_journal; |
| @@ -1407,7 +1402,6 @@ int jbd2_journal_forget (handle_t *handl |
| int err = 0; |
| int was_modified = 0; |
| |
| - WARN_ON(!transaction); |
| if (is_handle_aborted(handle)) |
| return -EROFS; |
| journal = transaction->t_journal; |
| @@ -1538,8 +1532,22 @@ int jbd2_journal_stop(handle_t *handle) |
| tid_t tid; |
| pid_t pid; |
| |
| - if (!transaction) |
| - goto free_and_exit; |
| + if (!transaction) { |
| + /* |
| + * Handle is already detached from the transaction so |
| + * there is nothing to do other than decrease a refcount, |
| + * or free the handle if refcount drops to zero |
| + */ |
| + if (--handle->h_ref > 0) { |
| + jbd_debug(4, "h_ref %d -> %d\n", handle->h_ref + 1, |
| + handle->h_ref); |
| + return err; |
| + } else { |
| + if (handle->h_rsv_handle) |
| + jbd2_free_handle(handle->h_rsv_handle); |
| + goto free_and_exit; |
| + } |
| + } |
| journal = transaction->t_journal; |
| |
| J_ASSERT(journal_current_handle() == handle); |
| @@ -2381,7 +2389,6 @@ int jbd2_journal_file_inode(handle_t *ha |
| transaction_t *transaction = handle->h_transaction; |
| journal_t *journal; |
| |
| - WARN_ON(!transaction); |
| if (is_handle_aborted(handle)) |
| return -EROFS; |
| journal = transaction->t_journal; |