On Wed, Dec 15, 2021 at 05:09:37PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> While I was running with KASAN and lockdep enabled, I stumbled upon an
> KASAN report about a UAF to a freed CIL checkpoint. Looking at the
> comment for xfs_log_item_in_current_chkpt, it seems pretty obvious to me
> that the original patch to xfs_defer_finish_noroll should have done
> something to lock the CIL to prevent it from switching the CIL contexts
> while the predicate runs.
>
> For upper level code that needs to know if a given log item is new
> enough not to need relogging, add a new wrapper that takes the CIL
> context lock long enough to sample the current CIL context. This is
> kind of racy in that the CIL can switch the contexts immediately after
> sampling, but that's ok because the consequence is that the defer ops
> code is a little slow to relog items.
>
I see the problem, but I don't think this is the right way to fix
it. The CIL context lock is already a major contention point in the
transaction commit code when it is only taken once per
xfs_trans_commit() call. If we now potentially take it once per
intent item per xfs_trans_commit() call, we're going to make the
contention even worse than it already is.
The current sequence is always available from the CIL itself via
cil->xc_current_sequence, and we can read that without needing any
locking to provide existence guarantees of the CIL structure.
So....
bool
xfs_log_item_in_current_chkpt(
struct xfs_log_item *lip)
{
struct xfs_cil *cil = lip->li_mountp->m_log->l_cilp;
if (list_empty(&lip->li_cil))
return false;
/*
* li_seq is written on the first commit of a log item to record the
* first checkpoint it is written to. Hence if it is different to the
* current sequence, we're in a new checkpoint.
*/
return lip->li_seq == READ_ONCE(cil->xc_current_sequence);
}
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
