On Wed, Jun 02, 2021 at 09:35:37AM -0700, Darrick J. Wong wrote:

On Wed, Jun 02, 2021 at 12:31:54PM -0400, Brian Foster wrote:

quoted

On Wed, Jun 02, 2021 at 09:02:38AM -0700, Darrick J. Wong wrote:

quoted

On Wed, Jun 02, 2021 at 09:32:34AM -0400, Brian Foster wrote:

quoted

On Thu, May 06, 2021 at 03:29:50PM -0400, Brian Foster wrote:

quoted

On Thu, May 06, 2021 at 12:56:11PM +1000, Dave Chinner wrote:

quoted

On Wed, May 05, 2021 at 07:50:17AM -0400, Brian Foster wrote:

quoted

On Tue, May 04, 2021 at 09:25:39AM +1000, Dave Chinner wrote:

quoted

On Mon, May 03, 2021 at 08:18:16AM -0400, Brian Foster wrote:
i.e. the problem here is that we've dropped the bip->bli_refcount
before we've locked the buffer and taken a reference to it for
the fail path?

OK, I see that xfs_buf_item_done() (called from ioend processing)
simply frees the buf log item and doesn't care about the bli
refcount at all. So the first ioend caller will free the buf log
item regardless of whether there are other references to it at all.

IOWs, once we unpin the buffer, the bli attached to the buffer and
being tracked in the AIL has -zero- references to the bli and so it
gets freed unconditionally on IO completion.

That seems to the be the problem here - the bli is not reference
counted while it is the AIL....

I think it depends on how you look at it. As you point out, we've had
this odd bli reference count pattern for as long as I can remember where

Yes, and it's been a constant source of use-after free bugs in
shutdown processing for as long as I can remember. I want to fix it
so we don't have to keep band-aiding this code every time we change
how some part of log item or stale inode/buffer processing works...

IME, most of the bugs in this area tend to be shutdown/error related and
generally relate to the complexity of all the various states and
contexts for the different callback contexts. That isn't a direct result
of this bli refcount behavior, as odd as it is, though that certainly
contributes to the overall complexity.

quoted

quoted

quoted

 
-	freed = atomic_dec_and_test(&bip->bli_refcount);
-
+	/*
+	 * We can wake pin waiters safely now because we still hold the
+	 * bli_refcount that was taken when the pin was gained.
+	 */
 	if (atomic_dec_and_test(&bp->b_pin_count))
 		wake_up_all(&bp->b_waiters);
 
-	if (freed && stale) {
-		ASSERT(bip->bli_flags & XFS_BLI_STALE);
-		ASSERT(xfs_buf_islocked(bp));
-		ASSERT(bp->b_flags & XBF_STALE);
-		ASSERT(bip->__bli_format.blf_flags & XFS_BLF_CANCEL);
-
-		trace_xfs_buf_item_unpin_stale(bip);
-
-		if (remove) {
-			/*
-			 * If we are in a transaction context, we have to
-			 * remove the log item from the transaction as we are
-			 * about to release our reference to the buffer.  If we
-			 * don't, the unlock that occurs later in
-			 * xfs_trans_uncommit() will try to reference the
-			 * buffer which we no longer have a hold on.
-			 */
-			if (!list_empty(&lip->li_trans))
-				xfs_trans_del_item(lip);
-
-			/*
-			 * Since the transaction no longer refers to the buffer,
-			 * the buffer should no longer refer to the transaction.
-			 */
-			bp->b_transp = NULL;
+	if (!stale) {
+		if (!remove) {
+			/* Nothing to do but drop the refcount the pin owned. */
+			atomic_dec(&bip->bli_refcount);
+			return;
 		}

Hmm.. this seems a bit wonky to me. This code historically acts on the
drop of the final reference to the bli.

Yes, and that's the problem that needs fixing. The AIL needs a
reference so that we aren't racing with writeback from the AIL to
free the object because both sets of code run without actually
holding an active reference to the BLI...

quoted

This is not critical for the
common (!stale && !remove) case because that's basically a no-op here
outside of dropping the reference, and it looks like the stale buffer
handling code further down continues to follow that model, but in this
branch it seems we're trying to be clever in how the reference is
managed and as a result can act on a bli that might actually have
additional references.

Who cares? If something else has active references, then we must not
free the bli or buffer here, anyway. The lack of active references
by active BLI usres is why we keep getting use-after-free bugs in
this code.....

Well, this impacts more than just whether we free the buffer or not in
the abort case. This potentially runs the I/O failure sequence on a
pinned buffer, or blocks log I/O completion on a buffer lock that might
be held by a transaction.

I don't know if these are immediate problems or not and this is all
abort/shutdown related, but re: my point above around shutdown issues,
I'd prefer to try and avoid these kind of oddball quirks if we can vs.
just replace the historical quirks with new ones.

quoted

quoted

If so, I don't think it's appropriate to run
through the error sequence that follows.

quoted

 
 		/*
-		 * If we get called here because of an IO error, we may or may
-		 * not have the item on the AIL. xfs_trans_ail_delete() will
-		 * take care of that situation. xfs_trans_ail_delete() drops
-		 * the AIL lock.
-		 */
-		if (bip->bli_flags & XFS_BLI_STALE_INODE) {
-			xfs_buf_item_done(bp);
-			xfs_buf_inode_iodone(bp);
-			ASSERT(list_empty(&bp->b_li_list));
-		} else {
-			xfs_trans_ail_delete(lip, SHUTDOWN_LOG_IO_ERROR);
-			xfs_buf_item_relse(bp);
-			ASSERT(bp->b_log_item == NULL);
-		}
-		xfs_buf_relse(bp);
-	} else if (freed && remove) {
-		/*
+		 * Fail the IO before we drop the bli refcount. This guarantees
+		 * that a racing writeback completion also failing the buffer
+		 * and running completion will not remove the last reference to
+		 * the bli and free it from under us.
+		 *
 		 * The buffer must be locked and held by the caller to simulate
 		 * an async I/O failure.
 		 */

@@ -559,7 +555,62 @@ xfs_buf_item_unpin(
 		xfs_buf_hold(bp);
 		bp->b_flags |= XBF_ASYNC;
 		xfs_buf_ioend_fail(bp);
+		xfs_buf_item_relse(bp);

Did you mean for this to be xfs_buf_item_put() instead of _relse()? The

Yes. I did say "untested" which implies the patch isn't complete or will
work. It's just a demonstration of how this reference counting might
be done, not a complete, working solution. Patches are a much faster
way of explaining the overall solution that plain text...

Sure, I'm just trying to clarify intent. It's a refcount patch so
whether we drop a refcount or explicitly free an objects is particularly
relevant. ;)

quoted

quoted

quoted

 	}
+
+	/*
+	 * Stale buffer - only process it if this is the last reference to the
+	 * BLI. If this is the last BLI reference, then the buffer will be
+	 * locked and have two references - once from the transaction commit and
+	 * one from the BLI - and we do not unlock and release transaction
+	 * reference until we've finished cleaning up the BLI.
+	 */
+	if (!atomic_dec_and_test(&bip->bli_refcount))
+		return;
+

If the buffer is stale, will this ever be the last reference now that
_item_committed() bumps the refcount?

If the AIL has a reference, then no.

Ok, but how would we get here without an AIL reference? That seems
impossible to me based on your patch.

quoted

quoted

This change also seems to have
ramifications for the code that follows, such as if a staled buffer is
already in the AIL (with a bli ref), would this code ever get to the
point of removing it?

That's easy enough to handle - if the buffer is stale and we are in
this code, we hold the buffer locked. Hence we can remove from the
AIL if it is in the AIL and drop that reference, too. Indeed, this
code already does the AIL removal, so all this requires is a simple
rearrangement of the logic in this function....

Hmm, I don't think that's a correct assertion. If the buffer is stale
and we're in this code and we have the last reference to the bli, then
we know we hold the buffer locked. Otherwise, ISTM we can get here while
the transaction that staled the buffer might still own the lock.

quoted

The only difference is that we have to do this before we drop the
current reference we have on the BLI, which is not what we do now
and that's where all the problems lie.

quoted

All in all, I'll reiterate that I think it would be nice to fix up the
bli reference count handling in general, but I think the scope and
complexity of that work is significantly beyond what is reasonably
necessary to fix this bug.

And so leaving the underlying problem in the code for the next set
of changes someone does to trigger the problem in a differen way.
We've indentified what the root cause is, so can we please spend
the time to fix it properly?

That is not what I'm saying at all. I'm saying that I'd prefer to fix
the bug first and then step back and evaluate the overall refcount
design independently because the latter is quite complex and there are
all kinds of subtle interactions that the RFC patch just glazes over (by
design). For example, how log recovery processes bli's slightly
differently looks like yet another impedence mismatch from when the fs
is fully active.

Just an update on this particular bit...

I've probably spent about a week and a half now working through some
attempts to rework the bli refcount handling (going back to the drawing
board at least a couple times) and while I can get to something mostly
functional (surviving an fstests run), the last steps to test/prove a
solid/reliable implementation end up falling short. On extended testing
I end up sorting through the same kind of shutdown/unmount hang, use
after free, extremely subtle interactions that I've spent many hours
trying to stamp out over the past several years.

Because of that, I don't think this is something worth pushing for in
the short term. While the proposed idea sounds nice in theory, my take
away in practice is that the current design is in place for a reason
(i.e. the refcount historically looks like a transaction reference count
used to provide unlocked/isolated access through the log subsystem as
opposed to a traditional memory object lifecycle reference count) and
has quite a lot of incremental stability that has been baked in over
time.

I do still think this is (or should be :P) ultimately fixable, but I'm
wondering if we're probably better served by exploring some of the
historical warts and subtle rules/dependencies/hurdles of the current
model to see if they can be removed or simplified to the point where the
reference counting incrementally and naturally becomes a bit more
straightforward. I probably need to reset my brain and think a little
more about that...

What do you want to do in the meantime?

How about: Elevate this patch from RFC to regular patch status (perhaps
with an XXX comment outlining the gap as a breadcrumb to remind future
us?) and merge that so that we at least fix the immediate UAF problem?

Well I had already sent a v1 [1] of the original UAF fix independent
from this work because I didn't want to bury the bug fix behind a
broader rework. I just wanted to follow up here because I've been
looking into the latter and this is where most of the discussion around
the design rework took place.

That said, I am currently looking at (yet another) alternative approach
that might be more in the direction of broadly cleaning things up vs.
stamping out an isolated problem. I just don't know how it will turn out
quite yet, so I could go either way between waiting on that and possibly
sending a v2, or going with v1 for now (assuming it's reviewed) and
basing subsequent cleanups on that. My instinct is that to leave a
record of an isolated fix is probably the proper thing to do regardless,
but I don't feel that strongly about it.

Aha, you /did/ send a v1.  I thought I had seen something like that.

Oh, I reviewed it too.  Hm.  Do you have any changes after v1.1, or do
you want me to pull that in as it is now?

I had no pending changes after the v1.1 tweak of patch 2. Thanks.

Brian

--D

quoted

Brian

[1] https://lore.kernel.org/linux-xfs/20210511135257.878743-1-bfoster@redhat.com/ (local)

quoted

I suspected that figuring out all the subtleties of the bli lifetimes
would be an intense effort.

--D

quoted

Brian

quoted

So I propose to rework my patch a bit into something that reflects the
intended direction (see below for an untested diff) and proceed from
there...

Brian

--- 8< ---

diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index fb69879e4b2b..7ff31788512b 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c

@@ -475,17 +475,8 @@ xfs_buf_item_pin(
 }
 
 /*
- * This is called to unpin the buffer associated with the buf log
- * item which was previously pinned with a call to xfs_buf_item_pin().
- *
- * Also drop the reference to the buf item for the current transaction.
- * If the XFS_BLI_STALE flag is set and we are the last reference,
- * then free up the buf log item and unlock the buffer.
- *
- * If the remove flag is set we are called from uncommit in the
- * forced-shutdown path.  If that is true and the reference count on
- * the log item is going to drop to zero we need to free the item's
- * descriptor in the transaction.
+ * This is called to unpin the buffer associated with the buf log item which
+ * was previously pinned with a call to xfs_buf_item_pin().
  */
 STATIC void
 xfs_buf_item_unpin(

@@ -502,12 +493,26 @@ xfs_buf_item_unpin(
 
 	trace_xfs_buf_item_unpin(bip);
 
+	/*
+	 * Drop the bli ref associated with the pin and grab the hold required
+	 * for the I/O simulation failure in the abort case. We have to do this
+	 * before the pin count drops because the AIL doesn't acquire a bli
+	 * reference. Therefore if the refcount drops to zero, the bli could
+	 * still be AIL resident and the buffer submitted for I/O (and freed on
+	 * completion) at any point before we return. This can be removed once
+	 * the AIL properly holds a reference on the bli.
+	 */
 	freed = atomic_dec_and_test(&bip->bli_refcount);
-
+	if (freed && !stale && remove)
+		xfs_buf_hold(bp);
 	if (atomic_dec_and_test(&bp->b_pin_count))
 		wake_up_all(&bp->b_waiters);
 
-	if (freed && stale) {
+	 /* nothing to do but drop the pin count if the bli is active */
+	if (!freed)
+		return;
+
+	if (stale) {
 		ASSERT(bip->bli_flags & XFS_BLI_STALE);
 		ASSERT(xfs_buf_islocked(bp));
 		ASSERT(bp->b_flags & XBF_STALE);

@@ -550,13 +555,13 @@ xfs_buf_item_unpin(
 			ASSERT(bp->b_log_item == NULL);
 		}
 		xfs_buf_relse(bp);
-	} else if (freed && remove) {
+	} else if (remove) {
 		/*
 		 * The buffer must be locked and held by the caller to simulate
-		 * an async I/O failure.
+		 * an async I/O failure. We acquired the hold for this case
+		 * before the buffer was unpinned.
 		 */
 		xfs_buf_lock(bp);
-		xfs_buf_hold(bp);
 		bp->b_flags |= XBF_ASYNC;
 		xfs_buf_ioend_fail(bp);
 	}