xfs
[Top] [All Lists]

Re: [PATCH] Fix use-after-free with buffers

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH] Fix use-after-free with buffers
From: Lachlan McIlroy <lachlan@xxxxxxx>
Date: Tue, 02 Sep 2008 16:02:47 +1000
Cc: xfs-dev <xfs-dev@xxxxxxx>, xfs-oss <xfs@xxxxxxxxxxx>
In-reply-to: <20080820125033.GA29680@xxxxxxxxxxxxx>
References: <48ABA9EC.5040902@xxxxxxx> <20080820125033.GA29680@xxxxxxxxxxxxx>
Reply-to: lachlan@xxxxxxx
User-agent: Thunderbird 2.0.0.16 (X11/20080707)
Christoph Hellwig wrote:
On Wed, Aug 20, 2008 at 03:21:48PM +1000, Lachlan McIlroy wrote:
        XB_TRACE(bp, "rele", bp->b_relse);

+       ASSERT(atomic_read(&bp->b_hold) > 0);
+
        if (unlikely(!hash)) {
                ASSERT(!bp->b_relse);
                if (atomic_dec_and_test(&bp->b_hold))
@@ -838,7 +840,6 @@ xfs_buf_rele(
                return;
        }

-       ASSERT(atomic_read(&bp->b_hold) > 0);

Makes sense, but how is this related to the other bits of the
patch?  All but log and iozero buffers should always be hashed.
Okay, not specifically related.  I'll split it out of this change.


@@ -732,6 +732,7 @@ xfs_buf_item_init(
        bip->bli_item.li_ops = &xfs_buf_item_ops;
        bip->bli_item.li_mountp = mp;
        bip->bli_buf = bp;
+       xfs_buf_hold(bp);

These refcount changes look good to me.

+void
+xfs_buf_item_free(
+       xfs_buf_log_item_t      *bip)
+{
+#ifdef XFS_TRANS_DEBUG
+       kmem_free(bip->bli_orig);
+       bip->bli_orig = NULL;
+       kmem_free(bip->bli_logged);
+       bip->bli_logged = NULL;
+#endif /* XFS_TRANS_DEBUG */
+
+#ifdef XFS_BLI_TRACE
+       ktrace_free(bip->bli_trace);
+#endif
+       kmem_zone_free(xfs_buf_item_zone, bip);
+}

Faktoring this out makes sense, although you might want to remove the
zeroing of bip->bli_orig and bip->bli_logged while you're at it, so that
slab poisoning can do it's work.
Okay, thanks, I'll post a V2.

<Prev in Thread] Current Thread [Next in Thread>
  • Re: [PATCH] Fix use-after-free with buffers, Lachlan McIlroy <=