[PATCH 0/2] xfs: fix a couple xfs_buf_log_item shutdown problems

[PATCH 0/2] xfs: fix a couple xfs_buf_log_item shutdown problems

[Brian Foster]

Hi all,

This series fixes a couple filesystem shutdown corner cases related to
dealing with the xfs_buf_log_item reference count. Patch 1 fixes a
reference count leak that leads to an unmount hang due to a held buffer.
Patch 2 fixes a premature free of the bli that leads to a panic due to
use after free. Both of the above problems can be reproduced by running
generic/388 in a loop (though it may require 100-200 iterations).

I was originally considering reworking the bli reference counting, but I
don't have a clear idea on whether a cleaner solution is available atm.
These isolated fixes are lower risk and resolve the associated problems,
so should probably be considered independently regardless.

With these patches applied, generic/388 survives several thousand
iterations and otherwise no regressions are observed in my tests.
Thoughts, reviews, flames appreciated.

Brian

Brian Foster (2):
  xfs: release bli from transaction properly on fs shutdown
  xfs: remove bli from AIL before release on transaction abort

 fs/xfs/xfs_buf_item.c  | 21 ++++++++++++---------
 fs/xfs/xfs_trans_buf.c | 21 ++++++++++++++-------
 2 files changed, 26 insertions(+), 16 deletions(-)

-- 
2.7.5

[PATCH 1/2] xfs: release bli from transaction properly on fs shutdown

[Brian Foster]

If a filesystem shutdown occurs with a buffer log item in the CIL
and a log force occurs, the ->iop_unpin() handler is generally
expected to tear down the bli properly. This entails freeing the bli
memory and releasing the associated hold on the buffer so it can be
released and the filesystem unmounted.

If this sequence occurs while ->bli_refcount is elevated (i.e.,
another transaction is open and attempting to modify the buffer),
however, ->iop_unpin() may not be responsible for releasing the bli.
Instead, the transaction may release the final ->bli_refcount
reference and thus xfs_trans_brelse() is responsible for tearing
down the bli.

While xfs_trans_brelse() does drop the reference count, it only
attempts to release the bli if it is clean (i.e., not in the
CIL/AIL). If the filesystem is shutdown and the bli is sitting dirty
in the CIL as noted above, this ends up skipping the last
opportunity to release the bli. In turn, this leaves the hold on the
buffer and causes an unmount hang. This can be reproduced by running
generic/388 in repetition.

Update xfs_trans_brelse() to handle this shutdown corner case
correctly. If the final bli reference is dropped and the filesystem
is shutdown, remove the bli from the AIL (if necessary) and release
the bli to drop the buffer hold and ensure an unmount does not hang.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_trans_buf.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
index 8ee29ca..86987d8 100644
--- a/fs/xfs/xfs_trans_buf.c
+++ b/fs/xfs/xfs_trans_buf.c
@@ -356,6 +356,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
 		 xfs_buf_t	*bp)
 {
 	xfs_buf_log_item_t	*bip;
+	int			freed;
 
 	/*
 	 * Default to a normal brelse() call if the tp is NULL.
@@ -419,16 +420,22 @@ xfs_trans_brelse(xfs_trans_t	*tp,
 	/*
 	 * Drop our reference to the buf log item.
 	 */
-	atomic_dec(&bip->bli_refcount);
+	freed = atomic_dec_and_test(&bip->bli_refcount);
 
 	/*
-	 * If the buf item is not tracking data in the log, then
-	 * we must free it before releasing the buffer back to the
-	 * free pool.  Before releasing the buffer to the free pool,
-	 * clear the transaction pointer in b_fsprivate2 to dissolve
-	 * its relation to this transaction.
+	 * If the buf item is not tracking data in the log, then we must free it
+	 * before releasing the buffer back to the free pool.
+	 *
+	 * If the fs has shutdown and we dropped the last reference, it may fall
+	 * on us to release a (possibly dirty) bli if it never made it to the
+	 * AIL (e.g., the aborted unpin already happened and didn't release it
+	 * due to our reference). Since we're already shutdown and need xa_lock,
+	 * just force remove from the AIL and release the bli here.
 	 */
-	if (!xfs_buf_item_dirty(bip)) {
+	if (XFS_FORCED_SHUTDOWN(tp->t_mountp) && freed) {
+		xfs_trans_ail_remove(&bip->bli_item, SHUTDOWN_LOG_IO_ERROR);
+		xfs_buf_item_relse(bp);
+	} else if (!xfs_buf_item_dirty(bip)) {
 /***
 		ASSERT(bp->b_pincount == 0);
 ***/
-- 
2.7.5

Re: [PATCH 1/2] xfs: release bli from transaction properly on fs shutdown

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 1/2] xfs: release bli from transaction properly on fs shutdown

[Carlos Maiolino]

On Tue, Jun 06, 2017 at 08:08:49AM -0400, Brian Foster wrote:
> If a filesystem shutdown occurs with a buffer log item in the CIL
> and a log force occurs, the ->iop_unpin() handler is generally
> expected to tear down the bli properly. This entails freeing the bli
> memory and releasing the associated hold on the buffer so it can be
> released and the filesystem unmounted.
> 

Looks good,

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

> If this sequence occurs while ->bli_refcount is elevated (i.e.,
> another transaction is open and attempting to modify the buffer),
> however, ->iop_unpin() may not be responsible for releasing the bli.
> Instead, the transaction may release the final ->bli_refcount
> reference and thus xfs_trans_brelse() is responsible for tearing
> down the bli.
> 
> While xfs_trans_brelse() does drop the reference count, it only
> attempts to release the bli if it is clean (i.e., not in the
> CIL/AIL). If the filesystem is shutdown and the bli is sitting dirty
> in the CIL as noted above, this ends up skipping the last
> opportunity to release the bli. In turn, this leaves the hold on the
> buffer and causes an unmount hang. This can be reproduced by running
> generic/388 in repetition.
> 
> Update xfs_trans_brelse() to handle this shutdown corner case
> correctly. If the final bli reference is dropped and the filesystem
> is shutdown, remove the bli from the AIL (if necessary) and release
> the bli to drop the buffer hold and ensure an unmount does not hang.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/xfs_trans_buf.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
> index 8ee29ca..86987d8 100644
> --- a/fs/xfs/xfs_trans_buf.c
> +++ b/fs/xfs/xfs_trans_buf.c
> @@ -356,6 +356,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
>  		 xfs_buf_t	*bp)
>  {
>  	xfs_buf_log_item_t	*bip;
> +	int			freed;
>  
>  	/*
>  	 * Default to a normal brelse() call if the tp is NULL.
> @@ -419,16 +420,22 @@ xfs_trans_brelse(xfs_trans_t	*tp,
>  	/*
>  	 * Drop our reference to the buf log item.
>  	 */
> -	atomic_dec(&bip->bli_refcount);
> +	freed = atomic_dec_and_test(&bip->bli_refcount);
>  
>  	/*
> -	 * If the buf item is not tracking data in the log, then
> -	 * we must free it before releasing the buffer back to the
> -	 * free pool.  Before releasing the buffer to the free pool,
> -	 * clear the transaction pointer in b_fsprivate2 to dissolve
> -	 * its relation to this transaction.
> +	 * If the buf item is not tracking data in the log, then we must free it
> +	 * before releasing the buffer back to the free pool.
> +	 *
> +	 * If the fs has shutdown and we dropped the last reference, it may fall
> +	 * on us to release a (possibly dirty) bli if it never made it to the
> +	 * AIL (e.g., the aborted unpin already happened and didn't release it
> +	 * due to our reference). Since we're already shutdown and need xa_lock,
> +	 * just force remove from the AIL and release the bli here.
>  	 */
> -	if (!xfs_buf_item_dirty(bip)) {
> +	if (XFS_FORCED_SHUTDOWN(tp->t_mountp) && freed) {
> +		xfs_trans_ail_remove(&bip->bli_item, SHUTDOWN_LOG_IO_ERROR);
> +		xfs_buf_item_relse(bp);
> +	} else if (!xfs_buf_item_dirty(bip)) {
>  /***
>  		ASSERT(bp->b_pincount == 0);
>  ***/
> -- 
> 2.7.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Carlos

[PATCH 2/2] xfs: remove bli from AIL before release on transaction abort

[Brian Foster]

When a buffer is modified, logged and committed, it ultimately ends
up sitting on the AIL with a dirty bli waiting for metadata
writeback. If another transaction locks and invalidates the buffer
(freeing an inode chunk, for example) in the meantime, the bli is
flagged as stale, the dirty state is cleared and the bli remains in
the AIL.

If a shutdown occurs before the transaction that has invalidated the
buffer is committed, the transaction is ultimately aborted. The log
items are flagged as such and ->iop_unlock() handles the aborted
items. Because the bli is clean (due to the invalidation),
->iop_unlock() unconditionally releases it. The log item may still
reside in the AIL, however, which means the I/O completion handler
may still run and attempt to access it. This results in assert
failure due to the release of the bli while still present in the AIL
and a subsequent NULL dereference and panic in the buffer I/O
completion handling. This can be reproduced by running generic/388
in repetition.

To avoid this problem, update xfs_buf_item_unlock() to first check
whether the bli is aborted and if so, remove it from the AIL before
it is released. This ensures that the bli is no longer accessed
during the shutdown sequence after it has been freed.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_buf_item.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 0306168..f6a8422 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -636,20 +636,23 @@ xfs_buf_item_unlock(
 
 	/*
 	 * Clean buffers, by definition, cannot be in the AIL. However, aborted
-	 * buffers may be dirty and hence in the AIL. Therefore if we are
-	 * aborting a buffer and we've just taken the last refernce away, we
-	 * have to check if it is in the AIL before freeing it. We need to free
-	 * it in this case, because an aborted transaction has already shut the
-	 * filesystem down and this is the last chance we will have to do so.
+	 * buffers may be in the AIL regardless of dirty state. An aborted
+	 * transaction that invalidates a buffer already in the AIL may have
+	 * marked it stale and cleared the dirty state, for example.
+	 *
+	 * Therefore if we are aborting a buffer and we've just taken the last
+	 * reference away, we have to check if it is in the AIL before freeing
+	 * it. We need to free it in this case, because an aborted transaction
+	 * has already shut the filesystem down and this is the last chance we
+	 * will have to do so.
 	 */
 	if (atomic_dec_and_test(&bip->bli_refcount)) {
-		if (clean)
-			xfs_buf_item_relse(bp);
-		else if (aborted) {
+		if (aborted) {
 			ASSERT(XFS_FORCED_SHUTDOWN(lip->li_mountp));
 			xfs_trans_ail_remove(lip, SHUTDOWN_LOG_IO_ERROR);
 			xfs_buf_item_relse(bp);
-		}
+		} else if (clean)
+			xfs_buf_item_relse(bp);
 	}
 
 	if (!(flags & XFS_BLI_HOLD))
-- 
2.7.5

Re: [PATCH 2/2] xfs: remove bli from AIL before release on transaction abort

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/2] xfs: remove bli from AIL before release on transaction abort

[Carlos Maiolino]

On Tue, Jun 06, 2017 at 08:08:50AM -0400, Brian Foster wrote:
> When a buffer is modified, logged and committed, it ultimately ends
> up sitting on the AIL with a dirty bli waiting for metadata
> writeback. If another transaction locks and invalidates the buffer
> (freeing an inode chunk, for example) in the meantime, the bli is
> flagged as stale, the dirty state is cleared and the bli remains in
> the AIL.
> 

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>


> If a shutdown occurs before the transaction that has invalidated the
> buffer is committed, the transaction is ultimately aborted. The log
> items are flagged as such and ->iop_unlock() handles the aborted
> items. Because the bli is clean (due to the invalidation),
> ->iop_unlock() unconditionally releases it. The log item may still
> reside in the AIL, however, which means the I/O completion handler
> may still run and attempt to access it. This results in assert
> failure due to the release of the bli while still present in the AIL
> and a subsequent NULL dereference and panic in the buffer I/O
> completion handling. This can be reproduced by running generic/388
> in repetition.
> 
> To avoid this problem, update xfs_buf_item_unlock() to first check
> whether the bli is aborted and if so, remove it from the AIL before
> it is released. This ensures that the bli is no longer accessed
> during the shutdown sequence after it has been freed.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/xfs_buf_item.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 0306168..f6a8422 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -636,20 +636,23 @@ xfs_buf_item_unlock(
>  
>  	/*
>  	 * Clean buffers, by definition, cannot be in the AIL. However, aborted
> -	 * buffers may be dirty and hence in the AIL. Therefore if we are
> -	 * aborting a buffer and we've just taken the last refernce away, we
> -	 * have to check if it is in the AIL before freeing it. We need to free
> -	 * it in this case, because an aborted transaction has already shut the
> -	 * filesystem down and this is the last chance we will have to do so.
> +	 * buffers may be in the AIL regardless of dirty state. An aborted
> +	 * transaction that invalidates a buffer already in the AIL may have
> +	 * marked it stale and cleared the dirty state, for example.
> +	 *
> +	 * Therefore if we are aborting a buffer and we've just taken the last
> +	 * reference away, we have to check if it is in the AIL before freeing
> +	 * it. We need to free it in this case, because an aborted transaction
> +	 * has already shut the filesystem down and this is the last chance we
> +	 * will have to do so.
>  	 */
>  	if (atomic_dec_and_test(&bip->bli_refcount)) {
> -		if (clean)
> -			xfs_buf_item_relse(bp);
> -		else if (aborted) {
> +		if (aborted) {
>  			ASSERT(XFS_FORCED_SHUTDOWN(lip->li_mountp));
>  			xfs_trans_ail_remove(lip, SHUTDOWN_LOG_IO_ERROR);
>  			xfs_buf_item_relse(bp);
> -		}
> +		} else if (clean)
> +			xfs_buf_item_relse(bp);
>  	}
>  
>  	if (!(flags & XFS_BLI_HOLD))
> -- 
> 2.7.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Carlos

Re: [PATCH 0/2] xfs: fix a couple xfs_buf_log_item shutdown problems

[Darrick J. Wong]

On Tue, Jun 06, 2017 at 08:08:48AM -0400, Brian Foster wrote:
> Hi all,
> 
> This series fixes a couple filesystem shutdown corner cases related to
> dealing with the xfs_buf_log_item reference count. Patch 1 fixes a
> reference count leak that leads to an unmount hang due to a held buffer.
> Patch 2 fixes a premature free of the bli that leads to a panic due to
> use after free. Both of the above problems can be reproduced by running
> generic/388 in a loop (though it may require 100-200 iterations).
> 
> I was originally considering reworking the bli reference counting, but I
> don't have a clear idea on whether a cleaner solution is available atm.
> These isolated fixes are lower risk and resolve the associated problems,
> so should probably be considered independently regardless.
> 
> With these patches applied, generic/388 survives several thousand
> iterations and otherwise no regressions are observed in my tests.
> Thoughts, reviews, flames appreciated.

Series looks ok, will test...
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

I'm not seeing anything in here that screams "hot 4.12 bug fix"; is that
impression correct?

--D

> 
> Brian
> 
> Brian Foster (2):
>   xfs: release bli from transaction properly on fs shutdown
>   xfs: remove bli from AIL before release on transaction abort
> 
>  fs/xfs/xfs_buf_item.c  | 21 ++++++++++++---------
>  fs/xfs/xfs_trans_buf.c | 21 ++++++++++++++-------
>  2 files changed, 26 insertions(+), 16 deletions(-)
> 
> -- 
> 2.7.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] xfs: fix a couple xfs_buf_log_item shutdown problems

[Brian Foster]

On Thu, Jun 08, 2017 at 08:56:53AM -0700, Darrick J. Wong wrote:
> On Tue, Jun 06, 2017 at 08:08:48AM -0400, Brian Foster wrote:
> > Hi all,
> > 
> > This series fixes a couple filesystem shutdown corner cases related to
> > dealing with the xfs_buf_log_item reference count. Patch 1 fixes a
> > reference count leak that leads to an unmount hang due to a held buffer.
> > Patch 2 fixes a premature free of the bli that leads to a panic due to
> > use after free. Both of the above problems can be reproduced by running
> > generic/388 in a loop (though it may require 100-200 iterations).
> > 
> > I was originally considering reworking the bli reference counting, but I
> > don't have a clear idea on whether a cleaner solution is available atm.
> > These isolated fixes are lower risk and resolve the associated problems,
> > so should probably be considered independently regardless.
> > 
> > With these patches applied, generic/388 survives several thousand
> > iterations and otherwise no regressions are observed in my tests.
> > Thoughts, reviews, flames appreciated.
> 
> Series looks ok, will test...
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> I'm not seeing anything in here that screams "hot 4.12 bug fix"; is that
> impression correct?
> 

Correct, thanks!

Brian

> --D
> 
> > 
> > Brian
> > 
> > Brian Foster (2):
> >   xfs: release bli from transaction properly on fs shutdown
> >   xfs: remove bli from AIL before release on transaction abort
> > 
> >  fs/xfs/xfs_buf_item.c  | 21 ++++++++++++---------
> >  fs/xfs/xfs_trans_buf.c | 21 ++++++++++++++-------
> >  2 files changed, 26 insertions(+), 16 deletions(-)
> > 
> > -- 
> > 2.7.5
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html