| Message ID | 1372009468-11651-1-git-send-email-tytso@mit.edu |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Sun 23-06-13 13:44:28, Ted Tso 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@mit.edu> > Reported-by: Younger Liu <younger.liu@huawei.com> > Cc: Jan Kara <jack@suse.cz> The patch looks good to me. So you can add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > 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 > @@ -368,6 +368,7 @@ repeat: > atomic_read(&transaction->t_outstanding_credits), > jbd2_log_space_left(journal)); > read_unlock(&journal->j_state_lock); > + current->journal_info = handle; > > lock_map_acquire(&handle->h_lockdep_map); > jbd2_journal_free_transaction(new_transaction); > @@ -442,14 +443,11 @@ handle_t *jbd2__journal_start(journal_t *journal, int nblocks, int rsv_blocks, > handle->h_rsv_handle = rsv_handle; > } > > - current->journal_info = handle; > - > err = start_this_handle(journal, handle, gfp_mask); > if (err < 0) { > if (handle->h_rsv_handle) > jbd2_free_handle(handle->h_rsv_handle); > jbd2_free_handle(handle); > - current->journal_info = NULL; > return ERR_PTR(err); > } > handle->h_type = type; > @@ -511,16 +509,13 @@ int jbd2_journal_start_reserved(handle_t *handle, unsigned int type, > } > > handle->h_journal = NULL; > - current->journal_info = handle; > /* > * GFP_NOFS is here because callers are likely from writeback or > * similarly constrained call sites > */ > ret = start_this_handle(journal, handle, GFP_NOFS); > - if (ret < 0) { > - current->journal_info = NULL; > + if (ret < 0) > jbd2_journal_free_reserved(handle); > - } > handle->h_type = type; > handle->h_line_no = line_no; > return ret; > @@ -555,6 +550,7 @@ int jbd2_journal_extend(handle_t *handle, int nblocks) > int wanted; > > result = -EIO; > + WARN_ON(!handle->h_transaction); > if (is_handle_aborted(handle)) > goto out; > > @@ -630,6 +626,7 @@ int jbd2__journal_restart(handle_t *handle, int nblocks, gfp_t gfp_mask) > tid_t tid; > int need_to_start, ret; > > + WARN_ON(!handle->h_transaction); > /* If we've had an abort of any type, don't even think about > * actually doing the restart! */ > if (is_handle_aborted(handle)) > @@ -654,6 +651,8 @@ 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; > + current->journal_info = NULL; > > jbd_debug(2, "restarting handle %p\n", handle); > need_to_start = !tid_geq(journal->j_commit_request, tid); > @@ -790,6 +789,7 @@ do_get_write_access(handle_t *handle, struct journal_head *jh, > int need_copy = 0; > unsigned long start_lock, time_lock; > > + WARN_ON(!handle->h_transaction); > if (is_handle_aborted(handle)) > return -EROFS; > > @@ -1057,6 +1057,7 @@ 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(!handle->h_transaction); > err = -EROFS; > if (is_handle_aborted(handle)) > goto out; > @@ -1269,6 +1270,7 @@ int jbd2_journal_dirty_metadata(handle_t *handle, struct buffer_head *bh) > struct journal_head *jh; > int ret = 0; > > + WARN_ON(!handle->h_transaction); > if (is_handle_aborted(handle)) > goto out; > jh = jbd2_journal_grab_journal_head(bh); > @@ -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 (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); > } > -- > 1.7.12.rc0.22.gcdd159b >
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@mit.edu> > Reported-by: Younger Liu <younger.liu@huawei.com> > Cc: Jan Kara <jack@suse.cz> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 @@ -368,6 +368,7 @@ repeat: atomic_read(&transaction->t_outstanding_credits), jbd2_log_space_left(journal)); read_unlock(&journal->j_state_lock); + current->journal_info = handle; lock_map_acquire(&handle->h_lockdep_map); jbd2_journal_free_transaction(new_transaction); @@ -442,14 +443,11 @@ handle_t *jbd2__journal_start(journal_t *journal, int nblocks, int rsv_blocks, handle->h_rsv_handle = rsv_handle; } - current->journal_info = handle; - err = start_this_handle(journal, handle, gfp_mask); if (err < 0) { if (handle->h_rsv_handle) jbd2_free_handle(handle->h_rsv_handle); jbd2_free_handle(handle); - current->journal_info = NULL; return ERR_PTR(err); } handle->h_type = type; @@ -511,16 +509,13 @@ int jbd2_journal_start_reserved(handle_t *handle, unsigned int type, } handle->h_journal = NULL; - current->journal_info = handle; /* * GFP_NOFS is here because callers are likely from writeback or * similarly constrained call sites */ ret = start_this_handle(journal, handle, GFP_NOFS); - if (ret < 0) { - current->journal_info = NULL; + if (ret < 0) jbd2_journal_free_reserved(handle); - } handle->h_type = type; handle->h_line_no = line_no; return ret; @@ -555,6 +550,7 @@ int jbd2_journal_extend(handle_t *handle, int nblocks) int wanted; result = -EIO; + WARN_ON(!handle->h_transaction); if (is_handle_aborted(handle)) goto out; @@ -630,6 +626,7 @@ int jbd2__journal_restart(handle_t *handle, int nblocks, gfp_t gfp_mask) tid_t tid; int need_to_start, ret; + WARN_ON(!handle->h_transaction); /* If we've had an abort of any type, don't even think about * actually doing the restart! */ if (is_handle_aborted(handle)) @@ -654,6 +651,8 @@ 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; + current->journal_info = NULL; jbd_debug(2, "restarting handle %p\n", handle); need_to_start = !tid_geq(journal->j_commit_request, tid); @@ -790,6 +789,7 @@ do_get_write_access(handle_t *handle, struct journal_head *jh, int need_copy = 0; unsigned long start_lock, time_lock; + WARN_ON(!handle->h_transaction); if (is_handle_aborted(handle)) return -EROFS; @@ -1057,6 +1057,7 @@ 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(!handle->h_transaction); err = -EROFS; if (is_handle_aborted(handle)) goto out; @@ -1269,6 +1270,7 @@ int jbd2_journal_dirty_metadata(handle_t *handle, struct buffer_head *bh) struct journal_head *jh; int ret = 0; + WARN_ON(!handle->h_transaction); if (is_handle_aborted(handle)) goto out; jh = jbd2_journal_grab_journal_head(bh); @@ -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 (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); }
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@mit.edu> Reported-by: Younger Liu <younger.liu@huawei.com> Cc: Jan Kara <jack@suse.cz> --- fs/jbd2/transaction.c | 27 ++++++++++++++++----------- include/linux/jbd2.h | 2 +- 2 files changed, 17 insertions(+), 12 deletions(-)