Re: [PATCH v2] xfs: prevent UAF in xfs_log_item_in_current_chkpt

[Dave Chinner <david@fromorbit.com>]

On Fri, Dec 17, 2021 at 09:45:00AM -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.
....
> 
> Fixes: 4e919af7827a ("xfs: periodically relog deferred intent items")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> v2: use READ_ONCE on CIL instead of taking locks to get cil ctx
> ---
>  fs/xfs/xfs_log_cil.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 6c93c8ada6f3..b59cc9c0961c 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -1442,9 +1442,9 @@ xlog_cil_force_seq(
>   */
>  bool
>  xfs_log_item_in_current_chkpt(
> -	struct xfs_log_item *lip)
> +	struct xfs_log_item	*lip)
>  {
> -	struct xfs_cil_ctx *ctx = lip->li_mountp->m_log->l_cilp->xc_ctx;
> +	struct xfs_cil		*cil = lip->li_mountp->m_log->l_cilp;
>  
>  	if (list_empty(&lip->li_cil))
>  		return false;
> @@ -1454,7 +1454,7 @@ xfs_log_item_in_current_chkpt(
>  	 * 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 == ctx->sequence;
> +	return lip->li_seq == READ_ONCE(cil->xc_current_sequence);
>  }

Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com