Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 63db7c81 authored by Brian Foster's avatar Brian Foster Committed by Darrick J. Wong
Browse files

xfs: use ->b_state to fix buffer I/O accounting release race 



We've had user reports of unmount hangs in xfs_wait_buftarg() that
analysis shows is due to btp->bt_io_count == -1. bt_io_count
represents the count of in-flight asynchronous buffers and thus
should always be >= 0. xfs_wait_buftarg() waits for this value to
stabilize to zero in order to ensure that all untracked (with
respect to the lru) buffers have completed I/O processing before
unmount proceeds to tear down in-core data structures.

The value of -1 implies an I/O accounting decrement race. Indeed,
the fact that xfs_buf_ioacct_dec() is called from xfs_buf_rele()
(where the buffer lock is no longer held) means that bp->b_flags can
be updated from an unsafe context. While a user-level reproducer is
currently not available, some intrusive hacks to run racing buffer
lookups/ioacct/releases from multiple threads was used to
successfully manufacture this problem.

Existing callers do not expect to acquire the buffer lock from
xfs_buf_rele(). Therefore, we can not safely update ->b_flags from
this context. It turns out that we already have separate buffer
state bits and associated serialization for dealing with buffer LRU
state in the form of ->b_state and ->b_lock. Therefore, replace the
_XBF_IN_FLIGHT flag with a ->b_state variant, update the I/O
accounting wrappers appropriately and make sure they are used with
the correct locking. This ensures that buffer in-flight state can be
modified at buffer release time without racing with modifications
from a buffer lock holder.

Fixes: 9c7504aa ("xfs: track and serialize in-flight async buffers against unmount")
Cc: <stable@vger.kernel.org> # v4.8+
Signed-off-by: default avatarBrian Foster <bfoster@redhat.com>
Reviewed-by: default avatarNikolay Borisov <nborisov@suse.com>
Tested-by: default avatarLibor Pechacek <lpechacek@suse.com>
Reviewed-by: default avatarDarrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: default avatarDarrick J. Wong <darrick.wong@oracle.com>
parent a54fba8f

fs/xfs/xfs_buf.c

+26 −12
Original line number Diff line number Diff line
@@ -97,28 +97,42 @@ static inline void 
xfs_buf_ioacct_inc(

	struct xfs_buf	*bp)

{

	if (bp->b_flags & (XBF_NO_IOACCT|_XBF_IN_FLIGHT))

	if (bp->b_flags & XBF_NO_IOACCT)

		return;



	ASSERT(bp->b_flags & XBF_ASYNC);

	bp->b_flags |= _XBF_IN_FLIGHT;

	spin_lock(&bp->b_lock);

	if (!(bp->b_state & XFS_BSTATE_IN_FLIGHT)) {

		bp->b_state |= XFS_BSTATE_IN_FLIGHT;

		percpu_counter_inc(&bp->b_target->bt_io_count);

	}

	spin_unlock(&bp->b_lock);

}



/*

 * Clear the in-flight state on a buffer about to be released to the LRU or

 * freed and unaccount from the buftarg.

 */

static inline void

xfs_buf_ioacct_dec(

__xfs_buf_ioacct_dec(

	struct xfs_buf	*bp)

{

	if (!(bp->b_flags & _XBF_IN_FLIGHT))

		return;

	ASSERT(spin_is_locked(&bp->b_lock));



	bp->b_flags &= ~_XBF_IN_FLIGHT;

	if (bp->b_state & XFS_BSTATE_IN_FLIGHT) {

		bp->b_state &= ~XFS_BSTATE_IN_FLIGHT;

		percpu_counter_dec(&bp->b_target->bt_io_count);

	}

}



static inline void

xfs_buf_ioacct_dec(

	struct xfs_buf	*bp)

{

	spin_lock(&bp->b_lock);

	__xfs_buf_ioacct_dec(bp);

	spin_unlock(&bp->b_lock);

}



/*

 * When we mark a buffer stale, we remove the buffer from the LRU and clear the
@@ -149,9 +163,9 @@ xfs_buf_stale( 
	 * unaccounted (released to LRU) before that occurs. Drop in-flight

	 * status now to preserve accounting consistency.

	 */

	xfs_buf_ioacct_dec(bp);



	spin_lock(&bp->b_lock);

	__xfs_buf_ioacct_dec(bp);



	atomic_set(&bp->b_lru_ref, 0);

	if (!(bp->b_state & XFS_BSTATE_DISPOSE) &&

	    (list_lru_del(&bp->b_target->bt_lru, &bp->b_lru)))
@@ -979,12 +993,12 @@ xfs_buf_rele( 
		 * ensures the decrement occurs only once per-buf.

		 */

		if ((atomic_read(&bp->b_hold) == 1) && !list_empty(&bp->b_lru))

			xfs_buf_ioacct_dec(bp);

			__xfs_buf_ioacct_dec(bp);

		goto out_unlock;

	}



	/* the last reference has been dropped ... */

	xfs_buf_ioacct_dec(bp);

	__xfs_buf_ioacct_dec(bp);

	if (!(bp->b_flags & XBF_STALE) && atomic_read(&bp->b_lru_ref)) {

		/*

		 * If the buffer is added to the LRU take a new reference to the

fs/xfs/xfs_buf.h

+2 −3
Original line number Diff line number Diff line
@@ -63,7 +63,6 @@ typedef enum { 
#define _XBF_KMEM	 (1 << 21)/* backed by heap memory */

#define _XBF_DELWRI_Q	 (1 << 22)/* buffer on a delwri queue */

#define _XBF_COMPOUND	 (1 << 23)/* compound buffer */

#define _XBF_IN_FLIGHT	 (1 << 25) /* I/O in flight, for accounting purposes */



typedef unsigned int xfs_buf_flags_t;
@@ -84,14 +83,14 @@ typedef unsigned int xfs_buf_flags_t; 
	{ _XBF_PAGES,		"PAGES" }, \

	{ _XBF_KMEM,		"KMEM" }, \

	{ _XBF_DELWRI_Q,	"DELWRI_Q" }, \

	{ _XBF_COMPOUND,	"COMPOUND" }, \

	{ _XBF_IN_FLIGHT,	"IN_FLIGHT" }

	{ _XBF_COMPOUND,	"COMPOUND" }





/*

 * Internal state flags.

 */

#define XFS_BSTATE_DISPOSE	 (1 << 0)	/* buffer being discarded */

#define XFS_BSTATE_IN_FLIGHT	 (1 << 1)	/* I/O in flight */



/*

 * The xfs_buftarg contains 2 notions of "sector size" -