xfs
[Top] [All Lists]

Re: [PATCH 50/50] xfs: use reference counts to free clean buffer items

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 50/50] xfs: use reference counts to free clean buffer items
From: Mark Tinguely <tinguely@xxxxxxx>
Date: Tue, 13 Aug 2013 10:03:19 -0500
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1376304611-22994-51-git-send-email-david@xxxxxxxxxxxxx>
References: <1376304611-22994-1-git-send-email-david@xxxxxxxxxxxxx> <1376304611-22994-51-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:9.0) Gecko/20120122 Thunderbird/9.0
On 08/12/13 05:50, Dave Chinner wrote:
From: Dave Chinner<dchinner@xxxxxxxxxx>

When a transaction is cancelled and the buffer log item is clean in
the transaction, the buffer log item is unconditionally freed. If
the log item is in the AIL, however, this leads to a use after free
condition as the item still has other users.

In this case, xfs_buf_item_relse() should only be called on clean
buffer items if the reference count has dropped to zero. This
ensures only the last user frees the item.

Signed-off-by: Dave Chinner<dchinner@xxxxxxxxxx>
---
  fs/xfs/xfs_buf_item.c | 6 ++----
  1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 9358504..3a944b1 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -613,11 +613,9 @@ xfs_buf_item_unlock(
                        }
                }
        }
-       if (clean)
-               xfs_buf_item_relse(bp);
-       else if (aborted) {
+       if (clean || aborted) {
                if (atomic_dec_and_test(&bip->bli_refcount)) {
-                       ASSERT(XFS_FORCED_SHUTDOWN(lip->li_mountp));
+                       ASSERT(!aborted || XFS_FORCED_SHUTDOWN(lip->li_mountp));
                        xfs_buf_item_relse(bp);
                }
        } else

why is a clean buffer on the AIL? Racing with a completion handler?

rather than this:

        if (clean || aborted) {
                if (atomic_dec_and_test(&bip->bli_refcount)) {
                        ASSERT(!aborted || XFS_FORCED_SHUTDOWN(lip->li_mountp));
                        xfs_buf_item_relse(bp);
                }
        } else
                atomic_dec(&bip->bli_refcount);

why not fold it into this:

        if (atomic_dec_and_test(&bip->bli_refcount)) {
                ASSERT(!aborted || XFS_FORCED_SHUTDOWN(lip->li_mountp));
                xfs_buf_item_relse(bp);
        }

--Mark.

<Prev in Thread] Current Thread [Next in Thread>