On 2013/6/24 1:44, Theodore Ts'o wrote:
> If jbd2_journal_restart() fails the handle will have been disconnected
> from the current transaction. In this situation, the handle must not
> be used for for any jbd2 function other than jbd2_journal_stop().
> Enforce this with by treating a handle which has a NULL transaction
> pointer as an aborted handle, and issue a kernel warning if
> jbd2_journal_extent(), jbd2_journal_get_write_access(),
> jbd2_journal_dirty_metadata(), etc. is called with an invalid handle.
>
> This commit also fixes a bug where jbd2_journal_stop() would trip over
> a kernel jbd2 assertion check when trying to free an invalid handle.
>
> Also move the responsibility of setting current->journal_info to
> start_this_handle(), simplifying the three users of this function.
>
> Signed-off-by: "Theodore Ts'o" <tytso@xxxxxxx>
> Reported-by: Younger Liu <younger.liu@xxxxxxxxxx>
> Cc: Jan Kara <jack@xxxxxxx>
Hi Ted,
There is a mistake in this patch.
Please see my comments below.
> ---
> fs/jbd2/transaction.c | 27 ++++++++++++++++-----------
> include/linux/jbd2.h | 2 +-
> 2 files changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> index 7391456..8ac8306 100644
> --- a/fs/jbd2/transaction.c
> +++ b/fs/jbd2/transaction.c
...
> @@ -1523,18 +1525,19 @@ int jbd2_journal_stop(handle_t *handle)
> {
> transaction_t *transaction = handle->h_transaction;
> journal_t *journal = transaction->t_journal;
> - int err, wait_for_commit = 0;
> + int err = 0, wait_for_commit = 0;
> tid_t tid;
> pid_t pid;
>
> + if (!handle->h_transaction)
> + goto free_and_exit;
> +
> J_ASSERT(journal_current_handle() == handle);
>
If jbd2__journal_restart fails, handle->h_transaction may be NULL.
So we should check handle->h_transaction before
"journal = transaction->t_journal", Right?
How about the following?
transaction_t *transaction = handle->h_transaction;
journal_t *journal = NULL;
int err = 0, wait_for_commit = 0;
tid_t tid;
pid_t pid;
if (!handle->h_transaction)
goto free_and_exit;
journal = transaction->t_journal;
J_ASSERT(journal_current_handle() == handle);
We should check this in other functions too,such as
jbd2_journal_extend(), jbd2_journal_get_create_access(),
jbd2_journal_dirty_metadata(),jbd2_journal_file_inode(),
jbd2__journal_restart(), etc.
> if (is_handle_aborted(handle))
> err = -EIO;
> - else {
> + else
> J_ASSERT(atomic_read(&transaction->t_updates) > 0);
> - err = 0;
> - }
>
> if (--handle->h_ref > 0) {
> jbd_debug(4, "h_ref %d -> %d\n", handle->h_ref + 1,
> @@ -1657,6 +1660,7 @@ int jbd2_journal_stop(handle_t *handle)
>
> if (handle->h_rsv_handle)
> jbd2_journal_free_reserved(handle->h_rsv_handle);
> +free_and_exit:
> jbd2_free_handle(handle);
> return err;
> }
> @@ -2364,6 +2368,7 @@ int jbd2_journal_file_inode(handle_t *handle, struct jbd2_inode *jinode)
> transaction_t *transaction = handle->h_transaction;
> journal_t *journal = transaction->t_journal;
>
> + WARN_ON(!handle->h_transaction);
> if (is_handle_aborted(handle))
> return -EIO;
>
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 5f3c094..1891112 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -1295,7 +1295,7 @@ static inline int is_journal_aborted(journal_t *journal)
>
> static inline int is_handle_aborted(handle_t *handle)
> {
> - if (handle->h_aborted)
> + if (handle->h_aborted || !handle->h_transaction)
> return 1;
> return is_journal_aborted(handle->h_transaction->t_journal);
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
