ocfs2: fix NULL h_transaction deref in ocfs2_assure_trans_credits

[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 [email protected]

Closes https://syzkaller.appspot.com/bug?extid=e9c15ff790cea6a0cfae

Cc [email protected]

Signed-off-by Ian Bridges <[email protected]>

---
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(-)

fs/ocfs2/journal.c +5 -1

index f9bf3bac085d..64a26da8eb28 100644
@@ -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;

Quoted 23 lines · 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.

Quoted 1 line

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

Quoted 116 lines

Reported-by [email protected]

Closes https://syzkaller.appspot.com/bug?extid=e9c15ff790cea6a0cfae

Cc [email protected]

Signed-off-by Ian Bridges <[email protected]>

---
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(-)

fs/ocfs2/journal.c +5 -1

index f9bf3bac085d..64a26da8eb28 100644
@@ -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;

Quoted 25 lines · Joseph Qi

Quoted 23 lines · 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.

Quoted 6 lines

Quoted 1 line

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

The pattern of the bug appears present in the commit before d647c5b2fbf8. (510a75028707).

The loop reuses one handle for every extent (fs/ocfs2/aops.c):

    handle = ocfs2_start_trans(osb, credits);
    ...
    list_for_each_entry(ue, &dwc->dw_zero_list, ue_node) {
        ret = ocfs2_assure_trans_credits(handle, credits);
        ...
        ret = ocfs2_mark_extent_written(inode, &et, handle, ...);
        ...
    }

ocfs2_mark_extent_written() reaches a restart that sets h_transaction to NULL. jbd2__journal_restart() (fs/jbd2/transaction.c) leaves it NULL when start_this_handle() fails on an aborted journal:

    stop_this_handle(handle);
    handle->h_transaction = NULL;
    ...
    ret = start_this_handle(journal, handle, gfp_mask);

ocfs2_extend_trans() (fs/ocfs2/journal.c) is what reaches that restart:

    status = jbd2_journal_extend(handle, nblocks, 0);
    ...
    if (status > 0) {
        ...
        status = jbd2_journal_restart(handle, ...);
        ...
    }

ocfs2_try_to_merge_extent() (fs/ocfs2/alloc.c) calls ocfs2_extend_trans() through ocfs2_extend_rotate_transaction() and swallows its error, so ocfs2_mark_extent_written() returns success with h_transaction left NULL:

    if (ctxt->c_split_covers_rec) {
        ret = ocfs2_extend_rotate_transaction(handle, 0,
                jbd2_handle_buffer_credits(handle), path);
        if (ret) {
            mlog_errno(ret);
            ret = 0;
            goto out;
        }
        ...
    }

The next ocfs2_assure_trans_credits() call in the loop then reads that NULL h_transaction and faults.

[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 [email protected]

Closes https://syzkaller.appspot.com/bug?extid=e9c15ff790cea6a0cfae

Cc [email protected]

Signed-off-by Ian Bridges <[email protected]>

---
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(-)

fs/ocfs2/journal.c +5 -1

index f9bf3bac085d..64a26da8eb28 100644
@@ -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;