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

[Joseph Qi <joseph.qi@linux.alibaba.com>]

On 6/13/26 8:16 PM, Ian Bridges wrote:
> On Fri, Jun 12, 2026 at 09:19:31AM +0800, Joseph Qi wrote:
>>
>> 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
> 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.
> 

Yes, you are right.

Thanks,
Joseph