From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kara Subject: Re: [PATCH 1/2] ext4: fix NULL pointer dereference when journal restart fails Date: Tue, 12 May 2015 14:24:30 +0200 Message-ID: <20150512122430.GA21068@quack.suse.cz> References: <1430899514-19128-1-git-send-email-lczerner@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: tytso@mit.edu, linux-ext4@vger.kernel.org To: Lukas Czerner Return-path: Received: from cantor2.suse.de ([195.135.220.15]:38228 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752910AbbELMYg (ORCPT ); Tue, 12 May 2015 08:24:36 -0400 Content-Disposition: inline In-Reply-To: <1430899514-19128-1-git-send-email-lczerner@redhat.com> Sender: linux-ext4-owner@vger.kernel.org List-ID: On Wed 06-05-15 10:05:13, Lukas Czerner wrote: > 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. However it's not _really_ aborted and we need to treat this > differently because an abort indicates critical error which might not be > the case here. > > So 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 other's up the call chain will still > reference the handle so we might potentially reference already freed > memory. > > I think that setting the h_transaction to NULL is just a hack and we can > do this properly by introducing h_stopped flag to the handle structure. No, it isn't a hack. It is actually clearly representing what has happened. By the time we set h_transaction to NULL, handle isn't associated with that transaction in any way (we decremented transaction->t_updates and thus the transaction can happily commit and be freed). So keeping h_transaction set is just hiding the real problem and possibly causing use-after-free issues. Now the problems you describe above are real. IMO we should treat failed jbd2_journal_restart() as a handle abort as close as possible - after all jbd2_journal_restart() can fail only in circumstances when we'd abort the handle anyway. Fixing jbd2_journal_stop() to handle refcount properly is easy enough. We could fix ext4_journal_stop() to just silently call jbd2_journal_stop() for handle that is already detached and thus avoid the NULL pointer dereference. If we really wanted to avoid NULL h_transaction, the clean fix for that would IMHO be to replace jbd2_journal_restart() with jbd2_journal_stop() and jbd2_journal_start() and return new handle from jbd2_journal_restart(). But that would need some benchmarking to verify it doesn't regress something and it would be a pain to propagate new handle out of all the callers of ext4_journal_restart(). Honza > Now we use h_stopped flag to indicate that the handle has already > been stopped so there is nothing else to do in jbd2_journal_stop() other > than decrease the refcount, or free the handle structure (in case refcount > drops to zero) and exit which exactly fits our case. So if we fail to start > the handle in jbd2__journal_restart() instead of setting h_transaction to > NULL we set the h_stopped flag and let the jbd2_journal_stop() deal with > this. > > This will also fix the NULL dereference because we no longer free the > h_transaction. > > Moreover remove all the WARN_ON's when we're dealing with already > stopped handle. Again the situation is quite similar with the aborted > handle and it is possible to get an already stopped handle, in this case > we do not want to emit an backtrace. > > Signed-off-by: Lukas Czerner > --- > fs/jbd2/transaction.c | 45 ++++++++++++++++++++++++++------------------- > include/linux/jbd2.h | 20 +++++++++++++++++++- > 2 files changed, 45 insertions(+), 20 deletions(-) > > diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c > index 5f09370..34bd0c5 100644 > --- a/fs/jbd2/transaction.c > +++ b/fs/jbd2/transaction.c > @@ -361,6 +361,7 @@ repeat: > handle->h_transaction = transaction; > handle->h_requested_credits = blocks; > handle->h_start_jiffies = jiffies; > + handle->h_stopped = 0; > atomic_inc(&transaction->t_updates); > atomic_inc(&transaction->t_handle_count); > jbd_debug(4, "Handle %p given %d credits (total %d, free %lu)\n", > @@ -551,8 +552,7 @@ int jbd2_journal_extend(handle_t *handle, int nblocks) > int result; > int wanted; > > - WARN_ON(!transaction); > - if (is_handle_aborted(handle)) > + if (is_handle_aborted_or_stopped(handle)) > return -EROFS; > journal = transaction->t_journal; > > @@ -627,11 +627,10 @@ int jbd2__journal_restart(handle_t *handle, int nblocks, gfp_t gfp_mask) > 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)) > - return 0; > + if (is_handle_aborted_or_stopped(handle)) > + return -EROFS; > journal = transaction->t_journal; > > /* > @@ -653,7 +652,7 @@ int jbd2__journal_restart(handle_t *handle, int nblocks, gfp_t gfp_mask) > wake_up(&journal->j_wait_updates); > tid = transaction->t_tid; > spin_unlock(&transaction->t_handle_lock); > - handle->h_transaction = NULL; > + jbd2_journal_stop_handle(handle); > current->journal_info = NULL; > > jbd_debug(2, "restarting handle %p\n", handle); > @@ -785,8 +784,7 @@ do_get_write_access(handle_t *handle, struct journal_head *jh, > int need_copy = 0; > unsigned long start_lock, time_lock; > > - WARN_ON(!transaction); > - if (is_handle_aborted(handle)) > + if (is_handle_aborted_or_stopped(handle)) > return -EROFS; > journal = transaction->t_journal; > > @@ -849,7 +847,7 @@ repeat: > unlock_buffer(bh); > > error = -EROFS; > - if (is_handle_aborted(handle)) { > + if (is_handle_aborted_or_stopped(handle)) { > jbd_unlock_bh_state(bh); > goto out; > } > @@ -1051,9 +1049,8 @@ int jbd2_journal_get_create_access(handle_t *handle, struct buffer_head *bh) > int err; > > jbd_debug(5, "journal_head %p\n", jh); > - WARN_ON(!transaction); > err = -EROFS; > - if (is_handle_aborted(handle)) > + if (is_handle_aborted_or_stopped(handle)) > goto out; > journal = transaction->t_journal; > err = 0; > @@ -1266,8 +1263,7 @@ int jbd2_journal_dirty_metadata(handle_t *handle, struct buffer_head *bh) > struct journal_head *jh; > int ret = 0; > > - WARN_ON(!transaction); > - if (is_handle_aborted(handle)) > + if (is_handle_aborted_or_stopped(handle)) > return -EROFS; > journal = transaction->t_journal; > jh = jbd2_journal_grab_journal_head(bh); > @@ -1397,8 +1393,7 @@ int jbd2_journal_forget (handle_t *handle, struct buffer_head *bh) > int err = 0; > int was_modified = 0; > > - WARN_ON(!transaction); > - if (is_handle_aborted(handle)) > + if (is_handle_aborted_or_stopped(handle)) > return -EROFS; > journal = transaction->t_journal; > > @@ -1530,8 +1525,19 @@ int jbd2_journal_stop(handle_t *handle) > tid_t tid; > pid_t pid; > > - if (!transaction) > - goto free_and_exit; > + if (is_handle_stopped(handle)) { > + /* > + * Handle is stopped 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 > + goto free_and_exit; > + } > journal = transaction->t_journal; > > J_ASSERT(journal_current_handle() == handle); > @@ -1665,7 +1671,9 @@ int jbd2_journal_stop(handle_t *handle) > > if (handle->h_rsv_handle) > jbd2_journal_free_reserved(handle->h_rsv_handle); > + > free_and_exit: > + jbd2_journal_stop_handle(handle); > jbd2_free_handle(handle); > return err; > } > @@ -2373,8 +2381,7 @@ int jbd2_journal_file_inode(handle_t *handle, struct jbd2_inode *jinode) > transaction_t *transaction = handle->h_transaction; > journal_t *journal; > > - WARN_ON(!transaction); > - if (is_handle_aborted(handle)) > + if (is_handle_aborted_or_stopped(handle)) > return -EROFS; > journal = transaction->t_journal; > > diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h > index 20e7f78..349975e 100644 > --- a/include/linux/jbd2.h > +++ b/include/linux/jbd2.h > @@ -441,6 +441,7 @@ struct jbd2_journal_handle > unsigned int h_jdata: 1; /* force data journaling */ > unsigned int h_reserved: 1; /* handle with reserved credits */ > unsigned int h_aborted: 1; /* fatal error on handle */ > + unsigned int h_stopped: 1; /* handle is already stopped */ > unsigned int h_type: 8; /* for handle statistics */ > unsigned int h_line_no: 16; /* for handle statistics */ > > @@ -1266,18 +1267,35 @@ static inline int is_journal_aborted(journal_t *journal) > return journal->j_flags & JBD2_ABORT; > } > > +static inline int is_handle_stopped(handle_t *handle) > +{ > + return handle->h_stopped; > +} > + > static inline int is_handle_aborted(handle_t *handle) > { > - if (handle->h_aborted || !handle->h_transaction) > + if (handle->h_aborted) > return 1; > return is_journal_aborted(handle->h_transaction->t_journal); > } > > +static inline int is_handle_aborted_or_stopped(handle_t *handle) > +{ > + if (is_handle_aborted(handle) || is_handle_stopped(handle)) > + return 1; > + return 0; > +} > + > static inline void jbd2_journal_abort_handle(handle_t *handle) > { > handle->h_aborted = 1; > } > > +static inline void jbd2_journal_stop_handle(handle_t *handle) > +{ > + handle->h_stopped = 1; > +} > + > #endif /* __KERNEL__ */ > > /* Comparison functions for transaction IDs: perform comparisons using > -- > 1.8.3.1 > > -- > 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 -- Jan Kara SUSE Labs, CR