| Message ID | 20180418154946.32006-1-tytso@mit.edu |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | ext4: set h_journal if there is a failure starting a reserved handle | expand |
> On Apr 18, 2018, at 9:49 AM, Theodore Ts'o <tytso@mit.edu> wrote: > > If ext4 tries to start a reserved handle via > jbd2_journal_start_reserved(), and the journal has been aborted, this > can result in a NULL pointer dereference. This is because the fields > h_journal and h_transaction in the handle structure share the same > meory, via a union, so jbd2_journal_start_reserved() will clear (typo) s/meory/memory/ Otherwise it looks fine. I was wondering if it would be safer to check if (journal == NULL) in jbd2_journal_free_reserved(), but that adds overhead for each caller, while your patch only adds the extra code in the error case. Reviewed-by: Andreas Dilger <adilger@dilger.ca> > h_journal before calling start_this_handle(). If this function fails > due to an aborted handle, h_journal will still be NULL, and the call > to jbd2_journal_free_reserved() will pass a NULL journal to > sub_reserve_credits(). > > This can be reproduced by running "kvm-xfstests -c dioread_nolock > generic/475". > > Fixes: 8f7d89f36829b ("jbd2: transaction reservation support") > Signed-off-by: Theodore Ts'o <tytso@mit.edu> > Cc: Jan Kara <jack@suse.cz> > Cc: stable@kernel.org # 3.11 > --- > fs/jbd2/transaction.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c > index ac311037d7a5..8aa453784402 100644 > --- a/fs/jbd2/transaction.c > +++ b/fs/jbd2/transaction.c > @@ -532,6 +532,7 @@ int jbd2_journal_start_reserved(handle_t *handle, unsigned int type, > */ > ret = start_this_handle(journal, handle, GFP_NOFS); > if (ret < 0) { > + handle->h_journal = journal; > jbd2_journal_free_reserved(handle); > return ret; > } > -- > 2.16.1.72.g5be1f00a9a > Cheers, Andreas
On Wed 18-04-18 11:49:46, Theodore Ts'o wrote: > If ext4 tries to start a reserved handle via > jbd2_journal_start_reserved(), and the journal has been aborted, this > can result in a NULL pointer dereference. This is because the fields > h_journal and h_transaction in the handle structure share the same > meory, via a union, so jbd2_journal_start_reserved() will clear > h_journal before calling start_this_handle(). If this function fails > due to an aborted handle, h_journal will still be NULL, and the call > to jbd2_journal_free_reserved() will pass a NULL journal to > sub_reserve_credits(). > > This can be reproduced by running "kvm-xfstests -c dioread_nolock > generic/475". > > Fixes: 8f7d89f36829b ("jbd2: transaction reservation support") > Signed-off-by: Theodore Ts'o <tytso@mit.edu> > Cc: Jan Kara <jack@suse.cz> > Cc: stable@kernel.org # 3.11 Thanks for fixing this! The patch looks good to me. You can add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > fs/jbd2/transaction.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c > index ac311037d7a5..8aa453784402 100644 > --- a/fs/jbd2/transaction.c > +++ b/fs/jbd2/transaction.c > @@ -532,6 +532,7 @@ int jbd2_journal_start_reserved(handle_t *handle, unsigned int type, > */ > ret = start_this_handle(journal, handle, GFP_NOFS); > if (ret < 0) { > + handle->h_journal = journal; > jbd2_journal_free_reserved(handle); > return ret; > } > -- > 2.16.1.72.g5be1f00a9a >
On Wed, Apr 18, 2018 at 10:07:43AM -0600, Andreas Dilger wrote: > > > On Apr 18, 2018, at 9:49 AM, Theodore Ts'o <tytso@mit.edu> wrote: > > > > If ext4 tries to start a reserved handle via > > jbd2_journal_start_reserved(), and the journal has been aborted, this > > can result in a NULL pointer dereference. This is because the fields > > h_journal and h_transaction in the handle structure share the same > > meory, via a union, so jbd2_journal_start_reserved() will clear > > (typo) s/meory/memory/ Thanks, fixed. - Ted
diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c index ac311037d7a5..8aa453784402 100644 --- a/fs/jbd2/transaction.c +++ b/fs/jbd2/transaction.c @@ -532,6 +532,7 @@ int jbd2_journal_start_reserved(handle_t *handle, unsigned int type, */ ret = start_this_handle(journal, handle, GFP_NOFS); if (ret < 0) { + handle->h_journal = journal; jbd2_journal_free_reserved(handle); return ret; }
If ext4 tries to start a reserved handle via jbd2_journal_start_reserved(), and the journal has been aborted, this can result in a NULL pointer dereference. This is because the fields h_journal and h_transaction in the handle structure share the same meory, via a union, so jbd2_journal_start_reserved() will clear h_journal before calling start_this_handle(). If this function fails due to an aborted handle, h_journal will still be NULL, and the call to jbd2_journal_free_reserved() will pass a NULL journal to sub_reserve_credits(). This can be reproduced by running "kvm-xfstests -c dioread_nolock generic/475". Fixes: 8f7d89f36829b ("jbd2: transaction reservation support") Signed-off-by: Theodore Ts'o <tytso@mit.edu> Cc: Jan Kara <jack@suse.cz> Cc: stable@kernel.org # 3.11 --- fs/jbd2/transaction.c | 1 + 1 file changed, 1 insertion(+)