[PATCH 0/4] xfs: fix CIL shutdown UAF and shutdown hang

[PATCH 0/4] xfs: fix CIL shutdown UAF and shutdown hang

[Dave Chinner]

Hi folks,

The following patches implement an initial fix for the UAF that can
occur in the CIL push code when a racing shutdown occurs. This was a
zero-day bug in the delayed logging code, and only recently
uncovered by the CIL pipelining changes that addresses a different
zero-day bug in the delayed logging code. This UAF exists regardless
in all kernels that support delayed logging (i.e. since 2.6.36), but
is extremely unlikely that anyone has hit it as it requires a
shutdown with extremely tight timing tolerances to trigger a UAF.

This is more of a problem for the current for-next tree, though,
because there is now a call to xlog_wait_on_iclog() in the UAF
window. While we don't reference the CIL context after the wait,
this will soon be needed to fix the /other/ zero-day problems found
by the CIL pipelining changes.

The encapsulation of the entire CIL commit iclog processing epilogue
in the icloglock effectively serialises this code against shutdown
races and allows us to error out before attaching the context to the
iclog if a shutdown has already occurred. Callbacks used to be under
the icloglock, but were split out in 2008 because of icloglock
contention causing log scalability problems (sound familiar? :).
Delayed logging fixed those icloglock scalability issues by moving
it out of the hot transaction commit path, so we can move the
callbacks back under the icloglock without re-introducing ancient
problems and solve the initial UAF problem this way.

With that problem solved, we can then fix the call to
xlog_wait_on_iclog() in the CIL push code by ensuring that it only
waits on older iclogs via LSN checks. As the wait drops the icloglock and
potentially re-opens us to the above UAF on shutdown, we have to be
careful not to reference the CIL context after the wait returns.

Hence the patches don't really fix the underlying cause of the
shutdown UAF here - this is intended as a low impact, easily
backportable solution to the problem. Work to fix the underlying
shutdown brokenness to remove the need to hold the icloglock from
callback attachment to xlog_state_release_iclog() is needed
(underway) before we can then apply start record ordering fixes and
re-introduce the CIL pipelining fixes and the rest of the CIL
scalabilty work....

Cheers,

Dave.

[PATCH 1/4] xfs: don't nest icloglock inside ic_callback_lock

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

It's completely unnecessary because callbacks are added to iclogs
without holding the icloglock, hence no amount of ordering between
the icloglock and ic_callback_lock will order the removal of
callbacks from the iclog.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index e93cac6b5378..bb4390942275 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2773,11 +2773,8 @@ static void
 xlog_state_do_iclog_callbacks(
 	struct xlog		*log,
 	struct xlog_in_core	*iclog)
-		__releases(&log->l_icloglock)
-		__acquires(&log->l_icloglock)
 {
 	trace_xlog_iclog_callbacks_start(iclog, _RET_IP_);
-	spin_unlock(&log->l_icloglock);
 	spin_lock(&iclog->ic_callback_lock);
 	while (!list_empty(&iclog->ic_callbacks)) {
 		LIST_HEAD(tmp);
@@ -2789,12 +2786,6 @@ xlog_state_do_iclog_callbacks(
 		spin_lock(&iclog->ic_callback_lock);
 	}
 
-	/*
-	 * Pick up the icloglock while still holding the callback lock so we
-	 * serialise against anyone trying to add more callbacks to this iclog
-	 * now we've finished processing.
-	 */
-	spin_lock(&log->l_icloglock);
 	spin_unlock(&iclog->ic_callback_lock);
 	trace_xlog_iclog_callbacks_done(iclog, _RET_IP_);
 }
@@ -2836,13 +2827,12 @@ xlog_state_do_callback(
 				iclog = iclog->ic_next;
 				continue;
 			}
+			spin_unlock(&log->l_icloglock);
 
-			/*
-			 * Running callbacks will drop the icloglock which means
-			 * we'll have to run at least one more complete loop.
-			 */
-			cycled_icloglock = true;
 			xlog_state_do_iclog_callbacks(log, iclog);
+			cycled_icloglock = true;
+
+			spin_lock(&log->l_icloglock);
 			if (XLOG_FORCED_SHUTDOWN(log))
 				wake_up_all(&iclog->ic_force_wait);
 			else
-- 
2.31.1

[PATCH 2/4] xfs: remove callback dequeue loop from xlog_state_do_iclog_callbacks

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

If we are processing callbacks on an iclog, nothing can be
concurrently adding callbacks to the loop. We only add callbacks to
the iclog when they are in ACTIVE or WANT_SYNC state, and we
explicitly do not add callbacks if the iclog is already in IOERROR
state.

The only way to have a dequeue racing with an enqueue is to be
processing a shutdown without a direct reference to an iclog in
ACTIVE or WANT_SYNC state. As the enqueue avoids this race
condition, we only ever need a single dequeue operation in
xlog_state_do_iclog_callbacks(). Hence we can remove the loop.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index bb4390942275..05b00fa4d661 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2774,19 +2774,15 @@ xlog_state_do_iclog_callbacks(
 	struct xlog		*log,
 	struct xlog_in_core	*iclog)
 {
-	trace_xlog_iclog_callbacks_start(iclog, _RET_IP_);
-	spin_lock(&iclog->ic_callback_lock);
-	while (!list_empty(&iclog->ic_callbacks)) {
-		LIST_HEAD(tmp);
+	LIST_HEAD(tmp);
 
-		list_splice_init(&iclog->ic_callbacks, &tmp);
-
-		spin_unlock(&iclog->ic_callback_lock);
-		xlog_cil_process_committed(&tmp);
-		spin_lock(&iclog->ic_callback_lock);
-	}
+	trace_xlog_iclog_callbacks_start(iclog, _RET_IP_);
 
+	spin_lock(&iclog->ic_callback_lock);
+	list_splice_init(&iclog->ic_callbacks, &tmp);
 	spin_unlock(&iclog->ic_callback_lock);
+
+	xlog_cil_process_committed(&tmp);
 	trace_xlog_iclog_callbacks_done(iclog, _RET_IP_);
 }
 
-- 
2.31.1

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

[Dave Chinner]

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.c b/fs/xfs/xfs_log.c
index 05b00fa4d661..c896c9041b8e 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1484,7 +1484,6 @@ xlog_alloc_log(
 		iclog->ic_state = XLOG_STATE_ACTIVE;
 		iclog->ic_log = log;
 		atomic_set(&iclog->ic_refcnt, 0);
-		spin_lock_init(&iclog->ic_callback_lock);
 		INIT_LIST_HEAD(&iclog->ic_callbacks);
 		iclog->ic_datap = (char *)iclog->ic_data + log->l_iclog_hsize;
 
@@ -2760,32 +2759,6 @@ xlog_state_iodone_process_iclog(
 	}
 }
 
-/*
- * Keep processing entries in the iclog callback list until we come around and
- * it is empty.  We need to atomically see that the list is empty and change the
- * state to DIRTY so that we don't miss any more callbacks being added.
- *
- * This function is called with the icloglock held and returns with it held. We
- * drop it while running callbacks, however, as holding it over thousands of
- * callbacks is unnecessary and causes excessive contention if we do.
- */
-static void
-xlog_state_do_iclog_callbacks(
-	struct xlog		*log,
-	struct xlog_in_core	*iclog)
-{
-	LIST_HEAD(tmp);
-
-	trace_xlog_iclog_callbacks_start(iclog, _RET_IP_);
-
-	spin_lock(&iclog->ic_callback_lock);
-	list_splice_init(&iclog->ic_callbacks, &tmp);
-	spin_unlock(&iclog->ic_callback_lock);
-
-	xlog_cil_process_committed(&tmp);
-	trace_xlog_iclog_callbacks_done(iclog, _RET_IP_);
-}
-
 STATIC void
 xlog_state_do_callback(
 	struct xlog		*log)
@@ -2814,6 +2787,8 @@ xlog_state_do_callback(
 		repeats++;
 
 		do {
+			LIST_HEAD(cb_list);
+
 			if (xlog_state_iodone_process_iclog(log, iclog,
 							&ioerror))
 				break;
@@ -2823,9 +2798,12 @@ xlog_state_do_callback(
 				iclog = iclog->ic_next;
 				continue;
 			}
+			list_splice_init(&iclog->ic_callbacks, &cb_list);
 			spin_unlock(&log->l_icloglock);
 
-			xlog_state_do_iclog_callbacks(log, iclog);
+			trace_xlog_iclog_callbacks_start(iclog, _RET_IP_);
+			xlog_cil_process_committed(&cb_list);
+			trace_xlog_iclog_callbacks_done(iclog, _RET_IP_);
 			cycled_icloglock = true;
 
 			spin_lock(&log->l_icloglock);
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
@@ -873,15 +873,21 @@ xlog_cil_push_work(
 
 	xfs_log_ticket_ungrant(log, tic);
 
-	spin_lock(&commit_iclog->ic_callback_lock);
+	/*
+	 * Once we attach the ctx to the iclog, a shutdown can process the
+	 * iclog, run the callbacks and free the ctx. The only thing preventing
+	 * this potential UAF situation here is that we are holding the
+	 * icloglock. Hence we cannot access the ctx after we have attached the
+	 * callbacks and dropped the icloglock.
+	 */
+	spin_lock(&log->l_icloglock);
 	if (commit_iclog->ic_state == XLOG_STATE_IOERROR) {
-		spin_unlock(&commit_iclog->ic_callback_lock);
+		spin_unlock(&log->l_icloglock);
 		goto out_abort;
 	}
 	ASSERT_ALWAYS(commit_iclog->ic_state == XLOG_STATE_ACTIVE ||
 		      commit_iclog->ic_state == XLOG_STATE_WANT_SYNC);
 	list_add_tail(&ctx->iclog_entry, &commit_iclog->ic_callbacks);
-	spin_unlock(&commit_iclog->ic_callback_lock);
 
 	/*
 	 * now the checkpoint commit is complete and we've attached the
@@ -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
+	 * the icloglock if we have to wait for completion of other iclogs.
 	 */
-	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

[PATCH 4/4] xfs: don't wait on future iclogs when pushing the CIL

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

The iclogbuf ring attached to the struct xlog is circular, hence the
first and last iclogs in the ring can only be determined by
comparing them against the log->l_iclog pointer.

In xfs_cil_push_work(), we want to wait on previous iclogs that were
issued so that we can flush them to stable storage with the commit
record write, and it simply waits on the previous iclog in the ring.
This, however, leads to CIL push hangs in generic/019 like so:

task:kworker/u33:0   state:D stack:12680 pid:    7 ppid:     2 flags:0x00004000
Workqueue: xfs-cil/pmem1 xlog_cil_push_work
Call Trace:
 __schedule+0x30b/0x9f0
 schedule+0x68/0xe0
 xlog_wait_on_iclog+0x121/0x190
 ? wake_up_q+0xa0/0xa0
 xlog_cil_push_work+0x994/0xa10
 ? _raw_spin_lock+0x15/0x20
 ? xfs_swap_extents+0x920/0x920
 process_one_work+0x1ab/0x390
 worker_thread+0x56/0x3d0
 ? rescuer_thread+0x3c0/0x3c0
 kthread+0x14d/0x170
 ? __kthread_bind_mask+0x70/0x70
 ret_from_fork+0x1f/0x30

With other threads blocking in either xlog_state_get_iclog_space()
waiting for iclog space or xlog_grant_head_wait() waiting for log
reservation space.

The problem here is that the previous iclog on the ring might
actually be a future iclog. That is, if log->l_iclog points at
commit_iclog, commit_iclog is the first (oldest) iclog in the ring
and there are no previous iclogs pending as they have all completed
their IO and been activated again. IOWs, commit_iclog->ic_prev
points to an iclog that will be written in the future, not one that
has been written in the past.

Hence, in this case, waiting on the ->ic_prev iclog is incorrect
behaviour, and depending on the state of the future iclog, we can
end up with a circular ABA wait cycle and we hang.

The fix is made more complex by the fact that many iclogs states
cannot be used to determine if the iclog is a past or future iclog.
Hence we have to determine past iclogs by checking the LSN of the
iclog rather than their state. A past ACTIVE iclog will have a LSN
of zero, while a future ACTIVE iclog will have a LSN greater than
the current iclog. We don't wait on either of these cases.

Similarly, a future iclog that hasn't completed IO will have an LSN
greater than the current iclog and so we don't wait on them. A past
iclog that is still undergoing IO completion will have a LSN less
than the current iclog and those are the only iclogs that we need to
wait on.

Hence we can use the iclog LSN to determine what iclogs we need to
wait on here.

Fixes: 5fd9256ce156 ("xfs: separate CIL commit record IO")
Reported-by: Brian Foster <bfoster@redhat.com>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log_cil.c | 35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index 27bed1d9cf29..83a932878177 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -877,7 +877,7 @@ xlog_cil_push_work(
 	 * Once we attach the ctx to the iclog, a shutdown can process the
 	 * iclog, run the callbacks and free the ctx. The only thing preventing
 	 * this potential UAF situation here is that we are holding the
-	 * icloglock. Hence we cannot access the ctx after we have attached the
+	 * icloglock. Hence we cannot access the ctx once we have attached the
 	 * callbacks and dropped the icloglock.
 	 */
 	spin_lock(&log->l_icloglock);
@@ -900,17 +900,38 @@ xlog_cil_push_work(
 	spin_unlock(&cil->xc_push_lock);
 
 	/*
-	 * If the checkpoint spans multiple iclogs, wait for all previous
-	 * 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.
+	 * If the checkpoint spans multiple iclogs, wait for all previous iclogs
+	 * to complete before we submit the commit_iclog. We can't use state
+	 * checks for this - ACTIVE can be either a past completed iclog or a
+	 * future iclog being filled, while WANT_SYNC through SYNC_DONE can be a
+	 * past or future iclog awaiting IO or ordered IO completion to be run.
+	 * In the latter case, if it's a future iclog and we wait on it, the we
+	 * will hang because it won't get processed through to ic_force_wait
+	 * wakeup until this commit_iclog is written to disk.  Hence we use the
+	 * iclog header lsn and compare it to the commit lsn to determine if we
+	 * need to wait on iclogs or not.
 	 *
 	 * NOTE: It is not safe reference the ctx after this check as we drop
 	 * the icloglock if we have to wait for completion of other iclogs.
 	 */
 	if (ctx->start_lsn != commit_lsn) {
-		xlog_wait_on_iclog(commit_iclog->ic_prev);
-		spin_lock(&log->l_icloglock);
+		xfs_lsn_t	plsn;
+
+		plsn = be64_to_cpu(commit_iclog->ic_prev->ic_header.h_lsn);
+		if (plsn && XFS_LSN_CMP(plsn, commit_lsn) < 0) {
+			/*
+			 * Waiting on ic_force_wait orders the completion of
+			 * iclogs older than ic_prev. Hence we only need to wait
+			 * on the most recent older iclog here.
+			 */
+			xlog_wait_on_iclog(commit_iclog->ic_prev);
+			spin_lock(&log->l_icloglock);
+		}
+
+		/*
+		 * We need to issue a pre-flush so that the ordering for this
+		 * checkpoint is correctly preserved down to stable storage.
+		 */
 		commit_iclog->ic_flags |= XLOG_ICL_NEED_FLUSH;
 	}
 
-- 
2.31.1

Re: [PATCH 1/4] xfs: don't nest icloglock inside ic_callback_lock

[Brian Foster]

On Tue, Jun 22, 2021 at 02:06:01PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> It's completely unnecessary because callbacks are added to iclogs
> without holding the icloglock, hence no amount of ordering between
> the icloglock and ic_callback_lock will order the removal of
> callbacks from the iclog.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_log.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index e93cac6b5378..bb4390942275 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2773,11 +2773,8 @@ static void
>  xlog_state_do_iclog_callbacks(
>  	struct xlog		*log,
>  	struct xlog_in_core	*iclog)
> -		__releases(&log->l_icloglock)
> -		__acquires(&log->l_icloglock)
>  {
>  	trace_xlog_iclog_callbacks_start(iclog, _RET_IP_);
> -	spin_unlock(&log->l_icloglock);
>  	spin_lock(&iclog->ic_callback_lock);
>  	while (!list_empty(&iclog->ic_callbacks)) {
>  		LIST_HEAD(tmp);
> @@ -2789,12 +2786,6 @@ xlog_state_do_iclog_callbacks(
>  		spin_lock(&iclog->ic_callback_lock);
>  	}
>  
> -	/*
> -	 * Pick up the icloglock while still holding the callback lock so we
> -	 * serialise against anyone trying to add more callbacks to this iclog
> -	 * now we've finished processing.
> -	 */

This makes sense wrt to the current locking, but I'd like to better
understand what's being removed. When would we add callbacks to an iclog
that's made it to this stage (i.e., already completed I/O)? Is this some
historical case or attempt at defensive logic?

Brian

> -	spin_lock(&log->l_icloglock);
>  	spin_unlock(&iclog->ic_callback_lock);
>  	trace_xlog_iclog_callbacks_done(iclog, _RET_IP_);
>  }
> @@ -2836,13 +2827,12 @@ xlog_state_do_callback(
>  				iclog = iclog->ic_next;
>  				continue;
>  			}
> +			spin_unlock(&log->l_icloglock);
>  
> -			/*
> -			 * Running callbacks will drop the icloglock which means
> -			 * we'll have to run at least one more complete loop.
> -			 */
> -			cycled_icloglock = true;
>  			xlog_state_do_iclog_callbacks(log, iclog);
> +			cycled_icloglock = true;
> +
> +			spin_lock(&log->l_icloglock);
>  			if (XLOG_FORCED_SHUTDOWN(log))
>  				wake_up_all(&iclog->ic_force_wait);
>  			else
> -- 
> 2.31.1
> 

Re: [PATCH 2/4] xfs: remove callback dequeue loop from xlog_state_do_iclog_callbacks

[Brian Foster]

On Tue, Jun 22, 2021 at 02:06:02PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> If we are processing callbacks on an iclog, nothing can be
> concurrently adding callbacks to the loop. We only add callbacks to
> the iclog when they are in ACTIVE or WANT_SYNC state, and we
> explicitly do not add callbacks if the iclog is already in IOERROR
> state.
> 
> The only way to have a dequeue racing with an enqueue is to be
> processing a shutdown without a direct reference to an iclog in
> ACTIVE or WANT_SYNC state. As the enqueue avoids this race
> condition, we only ever need a single dequeue operation in
> xlog_state_do_iclog_callbacks(). Hence we can remove the loop.
> 

This sort of relates to my question on the previous patch..

> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---

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

>  fs/xfs/xfs_log.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index bb4390942275..05b00fa4d661 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2774,19 +2774,15 @@ xlog_state_do_iclog_callbacks(
>  	struct xlog		*log,
>  	struct xlog_in_core	*iclog)
>  {
> -	trace_xlog_iclog_callbacks_start(iclog, _RET_IP_);
> -	spin_lock(&iclog->ic_callback_lock);
> -	while (!list_empty(&iclog->ic_callbacks)) {
> -		LIST_HEAD(tmp);
> +	LIST_HEAD(tmp);
>  
> -		list_splice_init(&iclog->ic_callbacks, &tmp);
> -
> -		spin_unlock(&iclog->ic_callback_lock);
> -		xlog_cil_process_committed(&tmp);
> -		spin_lock(&iclog->ic_callback_lock);
> -	}
> +	trace_xlog_iclog_callbacks_start(iclog, _RET_IP_);
>  
> +	spin_lock(&iclog->ic_callback_lock);
> +	list_splice_init(&iclog->ic_callbacks, &tmp);
>  	spin_unlock(&iclog->ic_callback_lock);
> +
> +	xlog_cil_process_committed(&tmp);
>  	trace_xlog_iclog_callbacks_done(iclog, _RET_IP_);
>  }
>  
> -- 
> 2.31.1
> 

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

[Brian Foster]

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
> 

Re: [PATCH 4/4] xfs: don't wait on future iclogs when pushing the CIL

[Brian Foster]

On Tue, Jun 22, 2021 at 02:06:04PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The iclogbuf ring attached to the struct xlog is circular, hence the
> first and last iclogs in the ring can only be determined by
> comparing them against the log->l_iclog pointer.
> 
> In xfs_cil_push_work(), we want to wait on previous iclogs that were
> issued so that we can flush them to stable storage with the commit
> record write, and it simply waits on the previous iclog in the ring.
> This, however, leads to CIL push hangs in generic/019 like so:
> 
> task:kworker/u33:0   state:D stack:12680 pid:    7 ppid:     2 flags:0x00004000
> Workqueue: xfs-cil/pmem1 xlog_cil_push_work
> Call Trace:
>  __schedule+0x30b/0x9f0
>  schedule+0x68/0xe0
>  xlog_wait_on_iclog+0x121/0x190
>  ? wake_up_q+0xa0/0xa0
>  xlog_cil_push_work+0x994/0xa10
>  ? _raw_spin_lock+0x15/0x20
>  ? xfs_swap_extents+0x920/0x920
>  process_one_work+0x1ab/0x390
>  worker_thread+0x56/0x3d0
>  ? rescuer_thread+0x3c0/0x3c0
>  kthread+0x14d/0x170
>  ? __kthread_bind_mask+0x70/0x70
>  ret_from_fork+0x1f/0x30
> 
> With other threads blocking in either xlog_state_get_iclog_space()
> waiting for iclog space or xlog_grant_head_wait() waiting for log
> reservation space.
> 
> The problem here is that the previous iclog on the ring might
> actually be a future iclog. That is, if log->l_iclog points at
> commit_iclog, commit_iclog is the first (oldest) iclog in the ring
> and there are no previous iclogs pending as they have all completed
> their IO and been activated again. IOWs, commit_iclog->ic_prev
> points to an iclog that will be written in the future, not one that
> has been written in the past.
> 
> Hence, in this case, waiting on the ->ic_prev iclog is incorrect
> behaviour, and depending on the state of the future iclog, we can
> end up with a circular ABA wait cycle and we hang.
> 
> The fix is made more complex by the fact that many iclogs states
> cannot be used to determine if the iclog is a past or future iclog.
> Hence we have to determine past iclogs by checking the LSN of the
> iclog rather than their state. A past ACTIVE iclog will have a LSN
> of zero, while a future ACTIVE iclog will have a LSN greater than
> the current iclog. We don't wait on either of these cases.
> 
> Similarly, a future iclog that hasn't completed IO will have an LSN
> greater than the current iclog and so we don't wait on them. A past
> iclog that is still undergoing IO completion will have a LSN less
> than the current iclog and those are the only iclogs that we need to
> wait on.
> 
> Hence we can use the iclog LSN to determine what iclogs we need to
> wait on here.
> 
> Fixes: 5fd9256ce156 ("xfs: separate CIL commit record IO")
> Reported-by: Brian Foster <bfoster@redhat.com>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---

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

>  fs/xfs/xfs_log_cil.c | 35 ++++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 27bed1d9cf29..83a932878177 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -877,7 +877,7 @@ xlog_cil_push_work(
>  	 * Once we attach the ctx to the iclog, a shutdown can process the
>  	 * iclog, run the callbacks and free the ctx. The only thing preventing
>  	 * this potential UAF situation here is that we are holding the
> -	 * icloglock. Hence we cannot access the ctx after we have attached the
> +	 * icloglock. Hence we cannot access the ctx once we have attached the
>  	 * callbacks and dropped the icloglock.
>  	 */
>  	spin_lock(&log->l_icloglock);
> @@ -900,17 +900,38 @@ xlog_cil_push_work(
>  	spin_unlock(&cil->xc_push_lock);
>  
>  	/*
> -	 * If the checkpoint spans multiple iclogs, wait for all previous
> -	 * 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.
> +	 * If the checkpoint spans multiple iclogs, wait for all previous iclogs
> +	 * to complete before we submit the commit_iclog. We can't use state
> +	 * checks for this - ACTIVE can be either a past completed iclog or a
> +	 * future iclog being filled, while WANT_SYNC through SYNC_DONE can be a
> +	 * past or future iclog awaiting IO or ordered IO completion to be run.
> +	 * In the latter case, if it's a future iclog and we wait on it, the we
> +	 * will hang because it won't get processed through to ic_force_wait
> +	 * wakeup until this commit_iclog is written to disk.  Hence we use the
> +	 * iclog header lsn and compare it to the commit lsn to determine if we
> +	 * need to wait on iclogs or not.
>  	 *
>  	 * NOTE: It is not safe reference the ctx after this check as we drop
>  	 * the icloglock if we have to wait for completion of other iclogs.
>  	 */
>  	if (ctx->start_lsn != commit_lsn) {
> -		xlog_wait_on_iclog(commit_iclog->ic_prev);
> -		spin_lock(&log->l_icloglock);
> +		xfs_lsn_t	plsn;
> +
> +		plsn = be64_to_cpu(commit_iclog->ic_prev->ic_header.h_lsn);
> +		if (plsn && XFS_LSN_CMP(plsn, commit_lsn) < 0) {
> +			/*
> +			 * Waiting on ic_force_wait orders the completion of
> +			 * iclogs older than ic_prev. Hence we only need to wait
> +			 * on the most recent older iclog here.
> +			 */
> +			xlog_wait_on_iclog(commit_iclog->ic_prev);
> +			spin_lock(&log->l_icloglock);
> +		}
> +
> +		/*
> +		 * We need to issue a pre-flush so that the ordering for this
> +		 * checkpoint is correctly preserved down to stable storage.
> +		 */
>  		commit_iclog->ic_flags |= XLOG_ICL_NEED_FLUSH;
>  	}
>  
> -- 
> 2.31.1
> 

Re: [PATCH 1/4] xfs: don't nest icloglock inside ic_callback_lock

[Dave Chinner]

On Tue, Jun 22, 2021 at 08:38:56AM -0400, Brian Foster wrote:
> On Tue, Jun 22, 2021 at 02:06:01PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > It's completely unnecessary because callbacks are added to iclogs
> > without holding the icloglock, hence no amount of ordering between
> > the icloglock and ic_callback_lock will order the removal of
> > callbacks from the iclog.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/xfs_log.c | 18 ++++--------------
> >  1 file changed, 4 insertions(+), 14 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> > index e93cac6b5378..bb4390942275 100644
> > --- a/fs/xfs/xfs_log.c
> > +++ b/fs/xfs/xfs_log.c
> > @@ -2773,11 +2773,8 @@ static void
> >  xlog_state_do_iclog_callbacks(
> >  	struct xlog		*log,
> >  	struct xlog_in_core	*iclog)
> > -		__releases(&log->l_icloglock)
> > -		__acquires(&log->l_icloglock)
> >  {
> >  	trace_xlog_iclog_callbacks_start(iclog, _RET_IP_);
> > -	spin_unlock(&log->l_icloglock);
> >  	spin_lock(&iclog->ic_callback_lock);
> >  	while (!list_empty(&iclog->ic_callbacks)) {
> >  		LIST_HEAD(tmp);
> > @@ -2789,12 +2786,6 @@ xlog_state_do_iclog_callbacks(
> >  		spin_lock(&iclog->ic_callback_lock);
> >  	}
> >  
> > -	/*
> > -	 * Pick up the icloglock while still holding the callback lock so we
> > -	 * serialise against anyone trying to add more callbacks to this iclog
> > -	 * now we've finished processing.
> > -	 */
> 
> This makes sense wrt to the current locking, but I'd like to better
> understand what's being removed. When would we add callbacks to an iclog
> that's made it to this stage (i.e., already completed I/O)? Is this some
> historical case or attempt at defensive logic?

This was done in 2008. It's very likely that, at the time, nobody
(including me) understood the iclog state machine well enough to
determine if we could race with adding iclogs at this time. Maybe
they did race and this was a bandaid over, say, a shutdown race condition.
But, more likely, it was just defensive to try to prevent callbacks
from being added before the iclog was marked ACTIVE again...

Really, though, nobody is going to be able to tell you why the code
was written like this in the first place because even the author
doesn't remember...

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/4] xfs: remove callback dequeue loop from xlog_state_do_iclog_callbacks

[Dave Chinner]

On Tue, Jun 22, 2021 at 08:39:07AM -0400, Brian Foster wrote:
> On Tue, Jun 22, 2021 at 02:06:02PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > If we are processing callbacks on an iclog, nothing can be
> > concurrently adding callbacks to the loop. We only add callbacks to
> > the iclog when they are in ACTIVE or WANT_SYNC state, and we
> > explicitly do not add callbacks if the iclog is already in IOERROR
> > state.
> > 
> > The only way to have a dequeue racing with an enqueue is to be
> > processing a shutdown without a direct reference to an iclog in
> > ACTIVE or WANT_SYNC state. As the enqueue avoids this race
> > condition, we only ever need a single dequeue operation in
> > xlog_state_do_iclog_callbacks(). Hence we can remove the loop.
> > 
> 
> This sort of relates to my question on the previous patch..

Been that way since 1995:

commit fdae46676ab5d359d02d955c989b20b18e2a97f8
Author: Adam Sweeney <ajs@sgi.com>
Date:   Thu May 4 20:54:43 1995 +0000

    275579 - - Fix timing bug in the log callback code.  Callbacks
    must be queued until the incore log buffer goes to the dirty
    state.

......
               /*
+                * Keep processing entries in the callback list
+                * until we come around and it is empty.  We need
+                * to atomically see that the list is empty and change
+                * the state to DIRTY so that we don't miss any more
+                * callbacks being added.
+                */
                spl = LOG_LOCK(log);
+               cb = iclog->ic_callback;
+               while (cb != 0) {
+                       iclog->ic_callback_tail = &(iclog->ic_callback);
+                       iclog->ic_callback = 0;
+                       LOG_UNLOCK(log, spl);
+
+                       /* perform callbacks in the order given */
+                       for (; cb != 0; cb = cb_next) {
+                               cb_next = cb->cb_next;
+                               cb->cb_func(cb->cb_arg);
+                       }
+                       spl = LOG_LOCK(log);
+                       cb = iclog->ic_callback;
+               }
+
+               ASSERT(iclog->ic_callback == 0);

THat's likely also what the locking I removed in the previous patch
was attempting to retain - atomic transition to DIRTY state -
without really understanding if it is necessary or not.

The only way I can see this happening now is racing with shutdown
state being set and callbacks being run at the same time a commit
record is being processed. But now we check for shutdown before we
add callbacks to the iclog, and hence we can't be adding callbacks
while shutdown state callbacks are being run. And that's made even
more impossible by this patch set that is serialising all the
shutdown state changes and callback add/remove under the same
lock....

So, yeah, this is largely behaviour that is historic and the
situation that it avoided is unknown and almost certainly doesn't
exist anymore...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/4] xfs: don't nest icloglock inside ic_callback_lock

[Brian Foster]

On Wed, Jun 23, 2021 at 08:42:47AM +1000, Dave Chinner wrote:
> On Tue, Jun 22, 2021 at 08:38:56AM -0400, Brian Foster wrote:
> > On Tue, Jun 22, 2021 at 02:06:01PM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > It's completely unnecessary because callbacks are added to iclogs
> > > without holding the icloglock, hence no amount of ordering between
> > > the icloglock and ic_callback_lock will order the removal of
> > > callbacks from the iclog.
> > > 
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > ---
> > >  fs/xfs/xfs_log.c | 18 ++++--------------
> > >  1 file changed, 4 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> > > index e93cac6b5378..bb4390942275 100644
> > > --- a/fs/xfs/xfs_log.c
> > > +++ b/fs/xfs/xfs_log.c
> > > @@ -2773,11 +2773,8 @@ static void
> > >  xlog_state_do_iclog_callbacks(
> > >  	struct xlog		*log,
> > >  	struct xlog_in_core	*iclog)
> > > -		__releases(&log->l_icloglock)
> > > -		__acquires(&log->l_icloglock)
> > >  {
> > >  	trace_xlog_iclog_callbacks_start(iclog, _RET_IP_);
> > > -	spin_unlock(&log->l_icloglock);
> > >  	spin_lock(&iclog->ic_callback_lock);
> > >  	while (!list_empty(&iclog->ic_callbacks)) {
> > >  		LIST_HEAD(tmp);
> > > @@ -2789,12 +2786,6 @@ xlog_state_do_iclog_callbacks(
> > >  		spin_lock(&iclog->ic_callback_lock);
> > >  	}
> > >  
> > > -	/*
> > > -	 * Pick up the icloglock while still holding the callback lock so we
> > > -	 * serialise against anyone trying to add more callbacks to this iclog
> > > -	 * now we've finished processing.
> > > -	 */
> > 
> > This makes sense wrt to the current locking, but I'd like to better
> > understand what's being removed. When would we add callbacks to an iclog
> > that's made it to this stage (i.e., already completed I/O)? Is this some
> > historical case or attempt at defensive logic?
> 
> This was done in 2008. It's very likely that, at the time, nobody
> (including me) understood the iclog state machine well enough to
> determine if we could race with adding iclogs at this time. Maybe
> they did race and this was a bandaid over, say, a shutdown race condition.
> But, more likely, it was just defensive to try to prevent callbacks
> from being added before the iclog was marked ACTIVE again...
> 
> Really, though, nobody is going to be able to tell you why the code
> was written like this in the first place because even the author
> doesn't remember...
> 

Ok, just wanted to be sure there wasn't some context I was missing. The
patch seems fine to me:

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

> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 

Re: [PATCH 1/4] xfs: don't nest icloglock inside ic_callback_lock

[Darrick J. Wong]

On Tue, Jun 22, 2021 at 02:06:01PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> It's completely unnecessary because callbacks are added to iclogs
> without holding the icloglock, hence no amount of ordering between
> the icloglock and ic_callback_lock will order the removal of
> callbacks from the iclog.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Seems reasonable so far -- we don't tangle the icloglock with the
callback lock anywhere like this.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_log.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index e93cac6b5378..bb4390942275 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2773,11 +2773,8 @@ static void
>  xlog_state_do_iclog_callbacks(
>  	struct xlog		*log,
>  	struct xlog_in_core	*iclog)
> -		__releases(&log->l_icloglock)
> -		__acquires(&log->l_icloglock)
>  {
>  	trace_xlog_iclog_callbacks_start(iclog, _RET_IP_);
> -	spin_unlock(&log->l_icloglock);
>  	spin_lock(&iclog->ic_callback_lock);
>  	while (!list_empty(&iclog->ic_callbacks)) {
>  		LIST_HEAD(tmp);
> @@ -2789,12 +2786,6 @@ xlog_state_do_iclog_callbacks(
>  		spin_lock(&iclog->ic_callback_lock);
>  	}
>  
> -	/*
> -	 * Pick up the icloglock while still holding the callback lock so we
> -	 * serialise against anyone trying to add more callbacks to this iclog
> -	 * now we've finished processing.
> -	 */
> -	spin_lock(&log->l_icloglock);
>  	spin_unlock(&iclog->ic_callback_lock);
>  	trace_xlog_iclog_callbacks_done(iclog, _RET_IP_);
>  }
> @@ -2836,13 +2827,12 @@ xlog_state_do_callback(
>  				iclog = iclog->ic_next;
>  				continue;
>  			}
> +			spin_unlock(&log->l_icloglock);
>  
> -			/*
> -			 * Running callbacks will drop the icloglock which means
> -			 * we'll have to run at least one more complete loop.
> -			 */
> -			cycled_icloglock = true;
>  			xlog_state_do_iclog_callbacks(log, iclog);
> +			cycled_icloglock = true;
> +
> +			spin_lock(&log->l_icloglock);
>  			if (XLOG_FORCED_SHUTDOWN(log))
>  				wake_up_all(&iclog->ic_force_wait);
>  			else
> -- 
> 2.31.1
> 

Re: [PATCH 2/4] xfs: remove callback dequeue loop from xlog_state_do_iclog_callbacks

[Darrick J. Wong]

On Tue, Jun 22, 2021 at 02:06:02PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> If we are processing callbacks on an iclog, nothing can be
> concurrently adding callbacks to the loop. We only add callbacks to
> the iclog when they are in ACTIVE or WANT_SYNC state, and we
> explicitly do not add callbacks if the iclog is already in IOERROR
> state.
> 
> The only way to have a dequeue racing with an enqueue is to be
> processing a shutdown without a direct reference to an iclog in
> ACTIVE or WANT_SYNC state. As the enqueue avoids this race
> condition, we only ever need a single dequeue operation in
> xlog_state_do_iclog_callbacks(). Hence we can remove the loop.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Makes sense...
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_log.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index bb4390942275..05b00fa4d661 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2774,19 +2774,15 @@ xlog_state_do_iclog_callbacks(
>  	struct xlog		*log,
>  	struct xlog_in_core	*iclog)
>  {
> -	trace_xlog_iclog_callbacks_start(iclog, _RET_IP_);
> -	spin_lock(&iclog->ic_callback_lock);
> -	while (!list_empty(&iclog->ic_callbacks)) {
> -		LIST_HEAD(tmp);
> +	LIST_HEAD(tmp);
>  
> -		list_splice_init(&iclog->ic_callbacks, &tmp);
> -
> -		spin_unlock(&iclog->ic_callback_lock);
> -		xlog_cil_process_committed(&tmp);
> -		spin_lock(&iclog->ic_callback_lock);
> -	}
> +	trace_xlog_iclog_callbacks_start(iclog, _RET_IP_);
>  
> +	spin_lock(&iclog->ic_callback_lock);
> +	list_splice_init(&iclog->ic_callbacks, &tmp);
>  	spin_unlock(&iclog->ic_callback_lock);
> +
> +	xlog_cil_process_committed(&tmp);
>  	trace_xlog_iclog_callbacks_done(iclog, _RET_IP_);
>  }
>  
> -- 
> 2.31.1
> 

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

[Darrick J. Wong]

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.c b/fs/xfs/xfs_log.c
> index 05b00fa4d661..c896c9041b8e 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -1484,7 +1484,6 @@ xlog_alloc_log(
>  		iclog->ic_state = XLOG_STATE_ACTIVE;
>  		iclog->ic_log = log;
>  		atomic_set(&iclog->ic_refcnt, 0);
> -		spin_lock_init(&iclog->ic_callback_lock);
>  		INIT_LIST_HEAD(&iclog->ic_callbacks);
>  		iclog->ic_datap = (char *)iclog->ic_data + log->l_iclog_hsize;
>  
> @@ -2760,32 +2759,6 @@ xlog_state_iodone_process_iclog(
>  	}
>  }
>  
> -/*
> - * Keep processing entries in the iclog callback list until we come around and
> - * it is empty.  We need to atomically see that the list is empty and change the
> - * state to DIRTY so that we don't miss any more callbacks being added.
> - *
> - * This function is called with the icloglock held and returns with it held. We
> - * drop it while running callbacks, however, as holding it over thousands of
> - * callbacks is unnecessary and causes excessive contention if we do.
> - */
> -static void
> -xlog_state_do_iclog_callbacks(
> -	struct xlog		*log,
> -	struct xlog_in_core	*iclog)
> -{
> -	LIST_HEAD(tmp);
> -
> -	trace_xlog_iclog_callbacks_start(iclog, _RET_IP_);
> -
> -	spin_lock(&iclog->ic_callback_lock);
> -	list_splice_init(&iclog->ic_callbacks, &tmp);
> -	spin_unlock(&iclog->ic_callback_lock);
> -
> -	xlog_cil_process_committed(&tmp);
> -	trace_xlog_iclog_callbacks_done(iclog, _RET_IP_);
> -}
> -
>  STATIC void
>  xlog_state_do_callback(
>  	struct xlog		*log)
> @@ -2814,6 +2787,8 @@ xlog_state_do_callback(
>  		repeats++;
>  
>  		do {
> +			LIST_HEAD(cb_list);
> +
>  			if (xlog_state_iodone_process_iclog(log, iclog,
>  							&ioerror))
>  				break;
> @@ -2823,9 +2798,12 @@ xlog_state_do_callback(
>  				iclog = iclog->ic_next;
>  				continue;
>  			}
> +			list_splice_init(&iclog->ic_callbacks, &cb_list);
>  			spin_unlock(&log->l_icloglock);
>  
> -			xlog_state_do_iclog_callbacks(log, iclog);
> +			trace_xlog_iclog_callbacks_start(iclog, _RET_IP_);
> +			xlog_cil_process_committed(&cb_list);
> +			trace_xlog_iclog_callbacks_done(iclog, _RET_IP_);
>  			cycled_icloglock = true;
>  
>  			spin_lock(&log->l_icloglock);
> 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
> @@ -873,15 +873,21 @@ xlog_cil_push_work(
>  
>  	xfs_log_ticket_ungrant(log, tic);
>  
> -	spin_lock(&commit_iclog->ic_callback_lock);
> +	/*
> +	 * Once we attach the ctx to the iclog, a shutdown can process the
> +	 * iclog, run the callbacks and free the ctx. The only thing preventing
> +	 * this potential UAF situation here is that we are holding the
> +	 * icloglock. Hence we cannot access the ctx after we have attached the
> +	 * callbacks and dropped the icloglock.
> +	 */
> +	spin_lock(&log->l_icloglock);
>  	if (commit_iclog->ic_state == XLOG_STATE_IOERROR) {
> -		spin_unlock(&commit_iclog->ic_callback_lock);
> +		spin_unlock(&log->l_icloglock);
>  		goto out_abort;
>  	}
>  	ASSERT_ALWAYS(commit_iclog->ic_state == XLOG_STATE_ACTIVE ||
>  		      commit_iclog->ic_state == XLOG_STATE_WANT_SYNC);
>  	list_add_tail(&ctx->iclog_entry, &commit_iclog->ic_callbacks);
> -	spin_unlock(&commit_iclog->ic_callback_lock);
>  
>  	/*
>  	 * now the checkpoint commit is complete and we've attached the
> @@ -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

"...not safe to reference..."

Hm, we're extending the icloglock to protect the callback list of an
iclog, which (afaict) looks safe since we require callers to hold the
icloglock to mess with any iclog's state.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> +	 * the icloglock if we have to wait for completion of other iclogs.
>  	 */
> -	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
> 

Re: [PATCH 4/4] xfs: don't wait on future iclogs when pushing the CIL

[Darrick J. Wong]

On Tue, Jun 22, 2021 at 02:06:04PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The iclogbuf ring attached to the struct xlog is circular, hence the
> first and last iclogs in the ring can only be determined by
> comparing them against the log->l_iclog pointer.
> 
> In xfs_cil_push_work(), we want to wait on previous iclogs that were
> issued so that we can flush them to stable storage with the commit
> record write, and it simply waits on the previous iclog in the ring.
> This, however, leads to CIL push hangs in generic/019 like so:
> 
> task:kworker/u33:0   state:D stack:12680 pid:    7 ppid:     2 flags:0x00004000
> Workqueue: xfs-cil/pmem1 xlog_cil_push_work
> Call Trace:
>  __schedule+0x30b/0x9f0
>  schedule+0x68/0xe0
>  xlog_wait_on_iclog+0x121/0x190
>  ? wake_up_q+0xa0/0xa0
>  xlog_cil_push_work+0x994/0xa10
>  ? _raw_spin_lock+0x15/0x20
>  ? xfs_swap_extents+0x920/0x920
>  process_one_work+0x1ab/0x390
>  worker_thread+0x56/0x3d0
>  ? rescuer_thread+0x3c0/0x3c0
>  kthread+0x14d/0x170
>  ? __kthread_bind_mask+0x70/0x70
>  ret_from_fork+0x1f/0x30
> 
> With other threads blocking in either xlog_state_get_iclog_space()
> waiting for iclog space or xlog_grant_head_wait() waiting for log
> reservation space.
> 
> The problem here is that the previous iclog on the ring might
> actually be a future iclog. That is, if log->l_iclog points at
> commit_iclog, commit_iclog is the first (oldest) iclog in the ring
> and there are no previous iclogs pending as they have all completed
> their IO and been activated again. IOWs, commit_iclog->ic_prev
> points to an iclog that will be written in the future, not one that
> has been written in the past.
> 
> Hence, in this case, waiting on the ->ic_prev iclog is incorrect
> behaviour, and depending on the state of the future iclog, we can
> end up with a circular ABA wait cycle and we hang.
> 
> The fix is made more complex by the fact that many iclogs states
> cannot be used to determine if the iclog is a past or future iclog.
> Hence we have to determine past iclogs by checking the LSN of the
> iclog rather than their state. A past ACTIVE iclog will have a LSN
> of zero, while a future ACTIVE iclog will have a LSN greater than
> the current iclog. We don't wait on either of these cases.
> 
> Similarly, a future iclog that hasn't completed IO will have an LSN
> greater than the current iclog and so we don't wait on them. A past
> iclog that is still undergoing IO completion will have a LSN less
> than the current iclog and those are the only iclogs that we need to
> wait on.
> 
> Hence we can use the iclog LSN to determine what iclogs we need to
> wait on here.
> 
> Fixes: 5fd9256ce156 ("xfs: separate CIL commit record IO")
> Reported-by: Brian Foster <bfoster@redhat.com>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Ok, looks good to me.
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_log_cil.c | 35 ++++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 27bed1d9cf29..83a932878177 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -877,7 +877,7 @@ xlog_cil_push_work(
>  	 * Once we attach the ctx to the iclog, a shutdown can process the
>  	 * iclog, run the callbacks and free the ctx. The only thing preventing
>  	 * this potential UAF situation here is that we are holding the
> -	 * icloglock. Hence we cannot access the ctx after we have attached the
> +	 * icloglock. Hence we cannot access the ctx once we have attached the
>  	 * callbacks and dropped the icloglock.
>  	 */
>  	spin_lock(&log->l_icloglock);
> @@ -900,17 +900,38 @@ xlog_cil_push_work(
>  	spin_unlock(&cil->xc_push_lock);
>  
>  	/*
> -	 * If the checkpoint spans multiple iclogs, wait for all previous
> -	 * 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.
> +	 * If the checkpoint spans multiple iclogs, wait for all previous iclogs
> +	 * to complete before we submit the commit_iclog. We can't use state
> +	 * checks for this - ACTIVE can be either a past completed iclog or a
> +	 * future iclog being filled, while WANT_SYNC through SYNC_DONE can be a
> +	 * past or future iclog awaiting IO or ordered IO completion to be run.
> +	 * In the latter case, if it's a future iclog and we wait on it, the we
> +	 * will hang because it won't get processed through to ic_force_wait
> +	 * wakeup until this commit_iclog is written to disk.  Hence we use the
> +	 * iclog header lsn and compare it to the commit lsn to determine if we
> +	 * need to wait on iclogs or not.
>  	 *
>  	 * NOTE: It is not safe reference the ctx after this check as we drop
>  	 * the icloglock if we have to wait for completion of other iclogs.
>  	 */
>  	if (ctx->start_lsn != commit_lsn) {
> -		xlog_wait_on_iclog(commit_iclog->ic_prev);
> -		spin_lock(&log->l_icloglock);
> +		xfs_lsn_t	plsn;
> +
> +		plsn = be64_to_cpu(commit_iclog->ic_prev->ic_header.h_lsn);
> +		if (plsn && XFS_LSN_CMP(plsn, commit_lsn) < 0) {
> +			/*
> +			 * Waiting on ic_force_wait orders the completion of
> +			 * iclogs older than ic_prev. Hence we only need to wait
> +			 * on the most recent older iclog here.
> +			 */
> +			xlog_wait_on_iclog(commit_iclog->ic_prev);
> +			spin_lock(&log->l_icloglock);
> +		}
> +
> +		/*
> +		 * We need to issue a pre-flush so that the ordering for this
> +		 * checkpoint is correctly preserved down to stable storage.
> +		 */
>  		commit_iclog->ic_flags |= XLOG_ICL_NEED_FLUSH;
>  	}
>  
> -- 
> 2.31.1
>