Date:	Tue, 12 May 2015 14:24:30 +0200
From:	Jan Kara <jack@...e.cz>
To:	Lukas Czerner <lczerner@...hat.com>
Cc:	tytso@....edu, linux-ext4@...r.kernel.org
Subject: Re: [PATCH 1/2] ext4: fix NULL pointer dereference when journal
 restart fails

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 <lczerner@...hat.com>
> ---
>  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@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Jan Kara <jack@...e.cz>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists