On Thu, Dec 16, 2021 at 03:36:07PM +1100, Dave Chinner wrote:
> 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);
Ooh, much better!
--D
> }
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
