Re: [PATCH 4/7] xfs: prevent UAF in xfs_log_item_in_current_chkpt

["Darrick J. Wong" <djwong@kernel.org>]

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@kernel.org>
> > 
> > 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@fromorbit.com