[PATCH] ocfs2: fix NULL h_transaction deref in ocfs2_assure_trans_credits

[PATCH] ocfs2: fix NULL h_transaction deref in ocfs2_assure_trans_credits

[Ian Bridges]

[BUG]
A direct write over unwritten extents can panic the kernel in
ocfs2_assure_trans_credits() when the journal aborts during DIO
completion. The crash is a general protection fault from a NULL pointer
dereference.

[CAUSE]
ocfs2_dio_end_io_write() loops over a direct write's unwritten extents,
marking each written under a single journal handle. If the journal
aborts (for example after an I/O error) while the extent tree is being
updated, the handle is left aborted with its transaction pointer
cleared. The extent merge treats that failure as not critical and
reports success, so the loop keeps using the handle.
ocfs2_assure_trans_credits() reads the handle's remaining credits
without first checking whether the handle is aborted, and that read
dereferences the cleared transaction pointer.

[FIX]
A journal abort is recorded in the handle itself, so callers are
expected to test the handle rather than rely on a returned error.
Make ocfs2_assure_trans_credits() do that, as the other ocfs2 journal
helpers already do, and return -EROFS when the handle is aborted.

Fixes: be346c1a6eeb ("ocfs2: fix DIO failure due to insufficient transaction credits")
Reported-by: syzbot+e9c15ff790cea6a0cfae@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=e9c15ff790cea6a0cfae
Cc: stable@vger.kernel.org
Signed-off-by: Ian Bridges <icb@fastmail.org>
---
This patch contains a proposed fix for a crash reported by syzbot
in ocfs2_assure_trans_credits().

The file names and offsets in this description are from commit
7cb1c5b32a2bfde961fff8d5204526b609bcb30a from this repo:
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git

I also have a small test harness that reproduces the original panic,
which I can make available as well.

The Bug

OCFS2 supports unwritten extents. These are ranges that fallocate()
has allocated on disk but flagged as holding no data, so reads return
zeros until the range is written. Clearing that flag is a journalled
metadata change. For a direct write, OCFS2 makes that change when the
write completes rather than when the write is submitted.

When a direct write to unwritten extents completes, ocfs2_dio_end_io()
(fs/ocfs2/aops.c:2401) calls ocfs2_dio_end_io_write()
(fs/ocfs2/aops.c:2266). That function opens one jbd2 handle and loops
over the extents the write covered (fs/ocfs2/aops.c:2319). For each one
it calls ocfs2_assure_trans_credits() (fs/ocfs2/aops.c:2334) and then
ocfs2_mark_extent_written() (fs/ocfs2/aops.c:2339). The same handle is
reused on each pass.

ocfs2_assure_trans_credits() (fs/ocfs2/journal.c:474) makes sure the
handle still has enough journal credits for the next extent operation.
Its first action is:

    int old_nblks = jbd2_handle_buffer_credits(handle);

jbd2_handle_buffer_credits() (include/linux/jbd2.h:1817) is an inline
accessor that reads handle->h_transaction->t_journal without a NULL
check. t_journal is at offset 0 of struct transaction_s, so a NULL
h_transaction makes this a read of address 0.

A handle's h_transaction is set to NULL when a transaction restart
fails. The bug is that ocfs2_dio_end_io_write() can keep using such a
handle. The sequence is:

1. ocfs2_mark_extent_written() reaches ocfs2_try_to_merge_extent()
   through ocfs2_change_extent_flag() and ocfs2_split_extent(). When the
   marked extent merges with a neighbor (ctxt->c_split_covers_rec,
   fs/ocfs2/alloc.c:3820), the merge reserves rotation credits with
   ocfs2_extend_rotate_transaction() (fs/ocfs2/alloc.c:3822), which calls
   ocfs2_extend_trans() (fs/ocfs2/journal.c:428).

2. ocfs2_extend_trans() cannot grow the running transaction, so it
   restarts the handle with jbd2_journal_restart()
   (fs/ocfs2/journal.c:454).

3. jbd2__journal_restart() (fs/jbd2/transaction.c) sets
   handle->h_transaction to NULL, then calls start_this_handle() to
   attach a new transaction. If the journal has aborted,
   start_this_handle() returns an error (fs/jbd2/transaction.c:366) and
   h_transaction stays NULL.

4. The error reaches ocfs2_try_to_merge_extent(), which ignores it.
   At fs/ocfs2/alloc.c:3827 it resets ret to 0 and returns success, so
   the loop does not stop.

5. The loop moves to the next extent and calls
   ocfs2_assure_trans_credits(handle) again (fs/ocfs2/aops.c:2334), now
   on the handle whose h_transaction is NULL.

6. ocfs2_assure_trans_credits() calls jbd2_handle_buffer_credits()
   (fs/ocfs2/journal.c:476), which dereferences the NULL h_transaction.
   This is the general protection fault syzbot reports.

The Proposed Fix

A failed transaction restart records the abort in the handle itself, as
a NULL h_transaction. It is not threaded back through return values, so
an intermediate caller that ignores the error, like
ocfs2_try_to_merge_extent() above, does not lose the abort. Each user is
instead expected to check the handle before touching it.

ocfs2 already does this. ocfs2_journal_dirty() (fs/ocfs2/journal.c:834)
and ocfs2_update_inode_fsync_trans() (fs/ocfs2/journal.h:603) both test
is_handle_aborted() before they read handle->h_transaction.

ocfs2_assure_trans_credits() is the one place that reads h_transaction,
through jbd2_handle_buffer_credits(), without that check. The fix adds
that check. is_handle_aborted() returns true when h_transaction is NULL,
so the NULL dereference cannot happen. Returning the error makes
ocfs2_dio_end_io_write() take its "goto commit" path and stop using the
handle.

 fs/ocfs2/journal.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
index f9bf3bac085d..64a26da8eb28 100644
--- a/fs/ocfs2/journal.c
+++ b/fs/ocfs2/journal.c
@@ -473,8 +473,12 @@ int ocfs2_extend_trans(handle_t *handle, int nblocks)
  */
 int ocfs2_assure_trans_credits(handle_t *handle, int nblocks)
 {
-	int old_nblks = jbd2_handle_buffer_credits(handle);
+	int old_nblks;
 
+	if (is_handle_aborted(handle))
+		return -EROFS;
+
+	old_nblks = jbd2_handle_buffer_credits(handle);
 	trace_ocfs2_assure_trans_credits(old_nblks);
 	if (old_nblks >= nblocks)
 		return 0;
-- 
2.47.3

Re: [PATCH] ocfs2: fix NULL h_transaction deref in ocfs2_assure_trans_credits

[Joseph Qi]

On 6/11/26 10:46 PM, Ian Bridges wrote:
> [BUG]
> A direct write over unwritten extents can panic the kernel in
> ocfs2_assure_trans_credits() when the journal aborts during DIO
> completion. The crash is a general protection fault from a NULL pointer
> dereference.
> 
> [CAUSE]
> ocfs2_dio_end_io_write() loops over a direct write's unwritten extents,
> marking each written under a single journal handle. If the journal
> aborts (for example after an I/O error) while the extent tree is being
> updated, the handle is left aborted with its transaction pointer
> cleared. The extent merge treats that failure as not critical and
> reports success, so the loop keeps using the handle.
> ocfs2_assure_trans_credits() reads the handle's remaining credits
> without first checking whether the handle is aborted, and that read
> dereferences the cleared transaction pointer.
> 
> [FIX]
> A journal abort is recorded in the handle itself, so callers are
> expected to test the handle rather than rely on a returned error.
> Make ocfs2_assure_trans_credits() do that, as the other ocfs2 journal
> helpers already do, and return -EROFS when the handle is aborted.
> 
> Fixes: be346c1a6eeb ("ocfs2: fix DIO failure due to insufficient transaction credits")

It seems this is introduced by commit d647c5b2fbf8 ("ocfs2: split
transactions in dio completion to avoid credit exhaustion")?

Thanks,
Joseph

> Reported-by: syzbot+e9c15ff790cea6a0cfae@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=e9c15ff790cea6a0cfae
> Cc: stable@vger.kernel.org
> Signed-off-by: Ian Bridges <icb@fastmail.org>
> ---
> This patch contains a proposed fix for a crash reported by syzbot
> in ocfs2_assure_trans_credits().
> 
> The file names and offsets in this description are from commit
> 7cb1c5b32a2bfde961fff8d5204526b609bcb30a from this repo:
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
> 
> I also have a small test harness that reproduces the original panic,
> which I can make available as well.
> 
> The Bug
> 
> OCFS2 supports unwritten extents. These are ranges that fallocate()
> has allocated on disk but flagged as holding no data, so reads return
> zeros until the range is written. Clearing that flag is a journalled
> metadata change. For a direct write, OCFS2 makes that change when the
> write completes rather than when the write is submitted.
> 
> When a direct write to unwritten extents completes, ocfs2_dio_end_io()
> (fs/ocfs2/aops.c:2401) calls ocfs2_dio_end_io_write()
> (fs/ocfs2/aops.c:2266). That function opens one jbd2 handle and loops
> over the extents the write covered (fs/ocfs2/aops.c:2319). For each one
> it calls ocfs2_assure_trans_credits() (fs/ocfs2/aops.c:2334) and then
> ocfs2_mark_extent_written() (fs/ocfs2/aops.c:2339). The same handle is
> reused on each pass.
> 
> ocfs2_assure_trans_credits() (fs/ocfs2/journal.c:474) makes sure the
> handle still has enough journal credits for the next extent operation.
> Its first action is:
> 
>     int old_nblks = jbd2_handle_buffer_credits(handle);
> 
> jbd2_handle_buffer_credits() (include/linux/jbd2.h:1817) is an inline
> accessor that reads handle->h_transaction->t_journal without a NULL
> check. t_journal is at offset 0 of struct transaction_s, so a NULL
> h_transaction makes this a read of address 0.
> 
> A handle's h_transaction is set to NULL when a transaction restart
> fails. The bug is that ocfs2_dio_end_io_write() can keep using such a
> handle. The sequence is:
> 
> 1. ocfs2_mark_extent_written() reaches ocfs2_try_to_merge_extent()
>    through ocfs2_change_extent_flag() and ocfs2_split_extent(). When the
>    marked extent merges with a neighbor (ctxt->c_split_covers_rec,
>    fs/ocfs2/alloc.c:3820), the merge reserves rotation credits with
>    ocfs2_extend_rotate_transaction() (fs/ocfs2/alloc.c:3822), which calls
>    ocfs2_extend_trans() (fs/ocfs2/journal.c:428).
> 
> 2. ocfs2_extend_trans() cannot grow the running transaction, so it
>    restarts the handle with jbd2_journal_restart()
>    (fs/ocfs2/journal.c:454).
> 
> 3. jbd2__journal_restart() (fs/jbd2/transaction.c) sets
>    handle->h_transaction to NULL, then calls start_this_handle() to
>    attach a new transaction. If the journal has aborted,
>    start_this_handle() returns an error (fs/jbd2/transaction.c:366) and
>    h_transaction stays NULL.
> 
> 4. The error reaches ocfs2_try_to_merge_extent(), which ignores it.
>    At fs/ocfs2/alloc.c:3827 it resets ret to 0 and returns success, so
>    the loop does not stop.
> 
> 5. The loop moves to the next extent and calls
>    ocfs2_assure_trans_credits(handle) again (fs/ocfs2/aops.c:2334), now
>    on the handle whose h_transaction is NULL.
> 
> 6. ocfs2_assure_trans_credits() calls jbd2_handle_buffer_credits()
>    (fs/ocfs2/journal.c:476), which dereferences the NULL h_transaction.
>    This is the general protection fault syzbot reports.
> 
> The Proposed Fix
> 
> A failed transaction restart records the abort in the handle itself, as
> a NULL h_transaction. It is not threaded back through return values, so
> an intermediate caller that ignores the error, like
> ocfs2_try_to_merge_extent() above, does not lose the abort. Each user is
> instead expected to check the handle before touching it.
> 
> ocfs2 already does this. ocfs2_journal_dirty() (fs/ocfs2/journal.c:834)
> and ocfs2_update_inode_fsync_trans() (fs/ocfs2/journal.h:603) both test
> is_handle_aborted() before they read handle->h_transaction.
> 
> ocfs2_assure_trans_credits() is the one place that reads h_transaction,
> through jbd2_handle_buffer_credits(), without that check. The fix adds
> that check. is_handle_aborted() returns true when h_transaction is NULL,
> so the NULL dereference cannot happen. Returning the error makes
> ocfs2_dio_end_io_write() take its "goto commit" path and stop using the
> handle.
> 
>  fs/ocfs2/journal.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
> index f9bf3bac085d..64a26da8eb28 100644
> --- a/fs/ocfs2/journal.c
> +++ b/fs/ocfs2/journal.c
> @@ -473,8 +473,12 @@ int ocfs2_extend_trans(handle_t *handle, int nblocks)
>   */
>  int ocfs2_assure_trans_credits(handle_t *handle, int nblocks)
>  {
> -	int old_nblks = jbd2_handle_buffer_credits(handle);
> +	int old_nblks;
>  
> +	if (is_handle_aborted(handle))
> +		return -EROFS;
> +
> +	old_nblks = jbd2_handle_buffer_credits(handle);
>  	trace_ocfs2_assure_trans_credits(old_nblks);
>  	if (old_nblks >= nblocks)
>  		return 0;