[PATCH 1/2] ext4: fix NULL pointer dereference when journal restart fails

[PATCH 1/2] ext4: fix NULL pointer dereference when journal restart fails

[Lukas Czerner]

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.

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@redhat.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

[PATCH 2/2] jbd2: Fix memory leak in jbd2_journal_stop()

[Lukas Czerner]

Currently in the case that we've got to the jbd2_journal_stop() with
already sopped handle we might leak the memory previously allocated
for the reserved handle because we only free the handle structure
leaving handle->h_rsv_handle intact.

Fix it by freeing the handle->h_rsv_handle structure in case we're
dealing with already stopped handle in jbd2_journal_stop().

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 fs/jbd2/transaction.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index 34bd0c5..807c378 100644
--- a/fs/jbd2/transaction.c
+++ b/fs/jbd2/transaction.c
@@ -1535,8 +1535,11 @@ int jbd2_journal_stop(handle_t *handle)
 			jbd_debug(4, "h_ref %d -> %d\n", handle->h_ref + 1,
 							 handle->h_ref);
 			return err;
-		} else
+		} else {
+			if (handle->h_rsv_handle)
+				jbd2_free_handle(handle->h_rsv_handle);
 			goto free_and_exit;
+		}
 	}
 	journal = transaction->t_journal;
 
-- 
1.8.3.1

Re: [PATCH 1/2] ext4: fix NULL pointer dereference when journal restart fails

[Jan Kara]

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@redhat.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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 1/2] ext4: fix NULL pointer dereference when journal restart fails

[Lukáš Czerner]

On Tue, 12 May 2015, Jan Kara wrote:

> Date: Tue, 12 May 2015 14:24:30 +0200
> From: Jan Kara <jack@suse.cz>
> To: Lukas Czerner <lczerner@redhat.com>
> Cc: tytso@mit.edu, linux-ext4@vger.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.

It's funny you say that since the current solution which is "not a
hack" has caused both NULL pointer dereference and use-after-free
issues :)

It was simply not expected by the code to get a h_transaction unset
among other problems. But I do understand your point and that we may
be asking for problems in the future by leaving h_transaction set
even though the handle has been disconnected from the transaction
and hence can be eventually freed.

I'll rework it.

Thanks!
-Lukas

> 
> 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@redhat.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@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>