[PATCH v2] xfs: avoid mount-time deadlock in CoW extent recovery

[PATCH v2] xfs: avoid mount-time deadlock in CoW extent recovery

[Darrick J. Wong]

If a malicious user corrupts the refcount btree to cause a cycle between
different levels of the tree, the next mount attempt will deadlock in
the CoW recovery routine while grabbing buffer locks.  We can use the
ability to re-grab a buffer that was previous locked to a transaction to
avoid deadlocks, so do that here.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: leave a comment discussing why we use an empty transaction
---
 fs/xfs/libxfs/xfs_refcount.c |   39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
index b177ef3..3d2b29b 100644
--- a/fs/xfs/libxfs/xfs_refcount.c
+++ b/fs/xfs/libxfs/xfs_refcount.c
@@ -1629,10 +1629,24 @@ xfs_refcount_recover_cow_leftovers(
 	if (mp->m_sb.sb_agblocks >= XFS_REFC_COW_START)
 		return -EOPNOTSUPP;
 
-	error = xfs_alloc_read_agf(mp, NULL, agno, 0, &agbp);
+	/*
+	 * In this first part, we use an empty transaction to gather up
+	 * all the leftover CoW extents so that we can subsequently
+	 * delete them.  The empty transaction is used to avoid
+	 * a buffer lock deadlock if there happens to be a loop in the
+	 * refcountbt because we're allowed to re-grab a buffer that is
+	 * already attached to our transaction.  When we're done
+	 * recording the CoW debris we cancel the (empty) transaction
+	 * and everything goes away cleanly.
+	 */
+	error = xfs_trans_alloc_empty(mp, &tp);
 	if (error)
 		return error;
-	cur = xfs_refcountbt_init_cursor(mp, NULL, agbp, agno, NULL);
+
+	error = xfs_alloc_read_agf(mp, tp, agno, 0, &agbp);
+	if (error)
+		goto out_trans;
+	cur = xfs_refcountbt_init_cursor(mp, tp, agbp, agno, NULL);
 
 	/* Find all the leftover CoW staging extents. */
 	INIT_LIST_HEAD(&debris);
@@ -1645,10 +1659,11 @@ xfs_refcount_recover_cow_leftovers(
 	if (error)
 		goto out_cursor;
 	xfs_btree_del_cursor(cur, XFS_BTREE_NOERROR);
-	xfs_buf_relse(agbp);
+	xfs_trans_brelse(tp, agbp);
+	xfs_trans_cancel(tp);
 
 	/* Now iterate the list to free the leftovers */
-	list_for_each_entry(rr, &debris, rr_list) {
+	list_for_each_entry_safe(rr, n, &debris, rr_list) {
 		/* Set up transaction. */
 		error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, 0, 0, 0, &tp);
 		if (error)
@@ -1676,8 +1691,15 @@ xfs_refcount_recover_cow_leftovers(
 		error = xfs_trans_commit(tp);
 		if (error)
 			goto out_free;
+
+		list_del(&rr->rr_list);
+		kmem_free(rr);
 	}
 
+	return error;
+out_defer:
+	xfs_defer_cancel(&dfops);
+	xfs_trans_cancel(tp);
 out_free:
 	/* Free the leftover list */
 	list_for_each_entry_safe(rr, n, &debris, rr_list) {
@@ -1688,11 +1710,8 @@ xfs_refcount_recover_cow_leftovers(
 
 out_cursor:
 	xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
-	xfs_buf_relse(agbp);
-	goto out_free;
-
-out_defer:
-	xfs_defer_cancel(&dfops);
+	xfs_trans_brelse(tp, agbp);
+out_trans:
 	xfs_trans_cancel(tp);
-	goto out_free;
+	return error;
 }

Re: [PATCH v2] xfs: avoid mount-time deadlock in CoW extent recovery

[Brian Foster]

On Tue, May 16, 2017 at 06:07:28PM -0700, Darrick J. Wong wrote:
> If a malicious user corrupts the refcount btree to cause a cycle between
> different levels of the tree, the next mount attempt will deadlock in
> the CoW recovery routine while grabbing buffer locks.  We can use the
> ability to re-grab a buffer that was previous locked to a transaction to
> avoid deadlocks, so do that here.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: leave a comment discussing why we use an empty transaction
> ---
>  fs/xfs/libxfs/xfs_refcount.c |   39 +++++++++++++++++++++++++++++----------
>  1 file changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
> index b177ef3..3d2b29b 100644
> --- a/fs/xfs/libxfs/xfs_refcount.c
> +++ b/fs/xfs/libxfs/xfs_refcount.c
...
> @@ -1676,8 +1691,15 @@ xfs_refcount_recover_cow_leftovers(
>  		error = xfs_trans_commit(tp);
>  		if (error)
>  			goto out_free;
> +
> +		list_del(&rr->rr_list);
> +		kmem_free(rr);
>  	}
>  
> +	return error;
> +out_defer:
> +	xfs_defer_cancel(&dfops);
> +	xfs_trans_cancel(tp);
>  out_free:
>  	/* Free the leftover list */
>  	list_for_each_entry_safe(rr, n, &debris, rr_list) {
> @@ -1688,11 +1710,8 @@ xfs_refcount_recover_cow_leftovers(
>  
>  out_cursor:
>  	xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
> -	xfs_buf_relse(agbp);
> -	goto out_free;
> -

Previously we had this jump to out_free here which presumably would
handle the case of a query range error with a populated list. Now it
looks like we just break down the cursor/transaction and return. Is that
case not possible?

Brian

> -out_defer:
> -	xfs_defer_cancel(&dfops);
> +	xfs_trans_brelse(tp, agbp);
> +out_trans:
>  	xfs_trans_cancel(tp);
> -	goto out_free;
> +	return error;
>  }
> --
> 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 v2] xfs: avoid mount-time deadlock in CoW extent recovery

[Darrick J. Wong]

On Thu, May 18, 2017 at 07:53:06AM -0400, Brian Foster wrote:
> On Tue, May 16, 2017 at 06:07:28PM -0700, Darrick J. Wong wrote:
> > If a malicious user corrupts the refcount btree to cause a cycle between
> > different levels of the tree, the next mount attempt will deadlock in
> > the CoW recovery routine while grabbing buffer locks.  We can use the
> > ability to re-grab a buffer that was previous locked to a transaction to
> > avoid deadlocks, so do that here.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > v2: leave a comment discussing why we use an empty transaction
> > ---
> >  fs/xfs/libxfs/xfs_refcount.c |   39 +++++++++++++++++++++++++++++----------
> >  1 file changed, 29 insertions(+), 10 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
> > index b177ef3..3d2b29b 100644
> > --- a/fs/xfs/libxfs/xfs_refcount.c
> > +++ b/fs/xfs/libxfs/xfs_refcount.c
> ...
> > @@ -1676,8 +1691,15 @@ xfs_refcount_recover_cow_leftovers(
> >  		error = xfs_trans_commit(tp);
> >  		if (error)
> >  			goto out_free;
> > +
> > +		list_del(&rr->rr_list);
> > +		kmem_free(rr);
> >  	}
> >  
> > +	return error;
> > +out_defer:
> > +	xfs_defer_cancel(&dfops);
> > +	xfs_trans_cancel(tp);
> >  out_free:
> >  	/* Free the leftover list */
> >  	list_for_each_entry_safe(rr, n, &debris, rr_list) {
> > @@ -1688,11 +1710,8 @@ xfs_refcount_recover_cow_leftovers(
> >  
> >  out_cursor:
> >  	xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
> > -	xfs_buf_relse(agbp);
> > -	goto out_free;
> > -
> 
> Previously we had this jump to out_free here which presumably would
> handle the case of a query range error with a populated list. Now it
> looks like we just break down the cursor/transaction and return. Is that
> case not possible?

Oops, yeah, I'll go refactor the exit paths again.  Thanks for catching this.

--D

> 
> Brian
> 
> > -out_defer:
> > -	xfs_defer_cancel(&dfops);
> > +	xfs_trans_brelse(tp, agbp);
> > +out_trans:
> >  	xfs_trans_cancel(tp);
> > -	goto out_free;
> > +	return error;
> >  }
> > --
> > 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
> --
> 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