Re: [PATCH 1/2] xfs: fix a use after free in xfs_defer_finish_recovery

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

On Thu, Dec 28, 2023 at 06:18:29AM +0000, Christoph Hellwig wrote:
> dfp will be freed by ->recover_work and thus the tracepoint in case
> of an error can lead to a use after free.
> 
> Store the defer ops in a local variable to avoid that.
> 
> Fixes: 7f2f7531e0d4 ("xfs: store an ops pointer in struct xfs_defer_pending")
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_defer.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index ca7f0ac0489604..785c92d2acaa73 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -915,12 +915,13 @@ xfs_defer_finish_recovery(
>  	struct xfs_defer_pending	*dfp,
>  	struct list_head		*capture_list)
>  {
> +	const struct xfs_defer_op_type	*ops = dfp->dfp_ops;
>  	int				error;
>  
> -	error = dfp->dfp_ops->recover_work(dfp, capture_list);
> +	error = ops->recover_work(dfp, capture_list);

Since I suck at remembering that dfp can be freed by recovery work, can
you add a comment to that effect? e.g.

	/* dfp is freed by recover_work and must not be accessed further */
	error = ops->recover_work(dfp, capture_list);

With that added,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D


>  	if (error)
>  		trace_xlog_intent_recovery_failed(mp, error,
> -				dfp->dfp_ops->recover_work);
> +				ops->recover_work);
>  	return error;
>  }
>  
> -- 
> 2.39.2
> 
>