xfs
[Top] [All Lists]

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

To: Lachlan McIlroy <lachlan@xxxxxxx>
Subject: Re: [PATCH] Fix use-after-free with buffers
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Wed, 20 Aug 2008 08:50:35 -0400
Cc: xfs-dev <xfs-dev@xxxxxxx>, xfs-oss <xfs@xxxxxxxxxxx>
In-reply-to: <48ABA9EC.5040902@xxxxxxx>
References: <48ABA9EC.5040902@xxxxxxx>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Mutt/1.5.18 (2008-05-17)
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.

> @@ -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.


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