Re: [PATCH 3/4] xfs: Fix a CIL UAF by getting get rid of the iclog callback lock

[Brian Foster <bfoster@redhat.com>]

On Tue, Jun 22, 2021 at 02:06:03PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The iclog callback chain has it's own lock. That was added way back
> in 2008 by myself to alleviate severe lock contention on the
> icloglock in commit 114d23aae512 ("[XFS] Per iclog callback chain
> lock"). This was long before delayed logging took the icloglock out
> of the hot transaction commit path and removed all contention on it.
> Hence the separate ic_callback_lock doesn't serve any scalability
> purpose anymore, and hasn't for close on a decade.
> 
> Further, we only attach callbacks to iclogs in one place where we
> are already taking the icloglock soon after attaching the callbacks.
> We also have to drop the icloglock to run callbacks and grab it
> immediately afterwards again. So given that the icloglock is no
> longer hot, making it cover callbacks again doesn't really change
> the locking patterns very much at all.
> 
> We also need to extend the icloglock to cover callback addition to
> fix a zero-day UAF in the CIL push code. This occurs when shutdown
> races with xlog_cil_push_work() and the shutdown runs the callbacks
> before the push releases the iclog. This results in the CIL context
> structure attached to the iclog being freed by the callback before
> the CIL push has finished referencing it, leading to UAF bugs.
> 
> Hence, to avoid this UAF, we need the callback attachment to be
> atomic with post processing of the commit iclog and references to
> the structures being attached to the iclog. This requires holding
> the icloglock as that's the only way to serialise iclog state
> against a shutdown in progress.
> 
> The result is we need to be using the icloglock to protect the
> callback list addition and removal and serialise them with shutdown.
> That makes the ic_callback_lock redundant and so it can be removed.
> 
> Fixes: 71e330b59390 ("xfs: Introduce delayed logging core code")
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_log.c      | 34 ++++++----------------------------
>  fs/xfs/xfs_log_cil.c  | 16 ++++++++++++----
>  fs/xfs/xfs_log_priv.h |  3 ---
>  3 files changed, 18 insertions(+), 35 deletions(-)
> 
...
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 3c2b1205944d..27bed1d9cf29 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
...
> @@ -898,8 +904,10 @@ xlog_cil_push_work(
>  	 * iclogs to complete before we submit the commit_iclog. In this case,
>  	 * the commit_iclog write needs to issue a pre-flush so that the
>  	 * ordering is correctly preserved down to stable storage.
> +	 *
> +	 * NOTE: It is not safe reference the ctx after this check as we drop

			   safe to reference

> +	 * the icloglock if we have to wait for completion of other iclogs.
>  	 */

Also, it's probably more clear to just say it's not safe to access the
ctx once we drop the lock since the conditional lock cycle is obvious
from the code. Otherwise:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> -	spin_lock(&log->l_icloglock);
>  	if (ctx->start_lsn != commit_lsn) {
>  		xlog_wait_on_iclog(commit_iclog->ic_prev);
>  		spin_lock(&log->l_icloglock);
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index 293d82b1fc0d..4c41bbfa33b0 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -216,9 +216,6 @@ typedef struct xlog_in_core {
>  	enum xlog_iclog_state	ic_state;
>  	unsigned int		ic_flags;
>  	char			*ic_datap;	/* pointer to iclog data */
> -
> -	/* Callback structures need their own cacheline */
> -	spinlock_t		ic_callback_lock ____cacheline_aligned_in_smp;
>  	struct list_head	ic_callbacks;
>  
>  	/* reference counts need their own cacheline */
> -- 
> 2.31.1
>