| Message ID | 20170517010728.GS4519@birch.djwong.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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 -- 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
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 -- 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
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; }
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(-) -- 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