On Tue, Jun 22, 2021 at 02:06:03PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
> - 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
>
