On 4/22/19 4:35 PM, Darrick J. Wong wrote:
> On Mon, Apr 22, 2019 at 04:26:58PM -0500, Eric Sandeen wrote:
>> On 4/22/19 10:45 AM, Darrick J. Wong wrote:
>>> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>>>
>>> Refactor the buffer item release code into a helper, which we will use
>>> in subsequent patches to make the buffer log item lifetime match the
>>> kernel equivalents.
>>>
>>> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>>> ---
>>> libxfs/trans.c | 14 +++++++++++---
>>> 1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>>
>>> diff --git a/libxfs/trans.c b/libxfs/trans.c
>>> index 9de77c8b..629501f8 100644
>>> --- a/libxfs/trans.c
>>> +++ b/libxfs/trans.c
>>> @@ -505,6 +505,16 @@ libxfs_trans_ordered_buf(
>>> return ret;
>>> }
>>>
>>> +static void
>>> +xfs_buf_item_put(
>>> + struct xfs_buf_log_item *bip)
>>> +{
>>> + struct xfs_buf *bp = bip->bli_buf;
>>> +
>>> + bp->b_log_item = NULL;
>>> + kmem_zone_free(xfs_buf_item_zone, bip);
>>> +}
>>> +
>>> void
>>> libxfs_trans_brelse(
>>> xfs_trans_t *tp,
>>> @@ -846,7 +856,6 @@ buf_item_done(
>>>
>>> bp = bip->bli_buf;
>>> ASSERT(bp != NULL);
>>> - bp->b_log_item = NULL; /* remove log item */
>>> bp->b_transp = NULL; /* remove xact ptr */
>>>
>>> hold = (bip->bli_flags & XFS_BLI_HOLD);
>>> @@ -861,8 +870,7 @@ buf_item_done(
>>> bip->bli_flags &= ~XFS_BLI_HOLD;
>>> else
>>> libxfs_putbuf(bp);
>>> - /* release the buf item */
>>> - kmem_zone_free(xfs_buf_item_zone, bip);
>>> + xfs_buf_item_put(bip);
>>
>> In xfs_buf_item_put(), we reach back up from bip to bip->bli_buf, which is
>> the bp. This is after we did a libxfs_putbuf(bp) on that bp. Is there not
>> a chance of use after free here? Enough puts and a shaker can run, right?
>
> I think you're right, the xfs_buf_item_put should come before the
> libxfs_putbuf.
Yeah in the kernel, it comes before xfs_buf_relse, and in userspace,
#define xfs_buf_relse(bp) libxfs_putbuf(bp)
-Eric
