diff mbox series

[07/10] libxfs: refactor buffer item release code

Message ID 155594793429.115924.9115512760848857551.stgit@magnolia (mailing list archive)
State Accepted, archived
Headers show
Series xfsprogs-5.0: fix various problems | expand

Commit Message

Darrick J. Wong April 22, 2019, 3:45 p.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

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@oracle.com>
---
 libxfs/trans.c |   14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Comments

Eric Sandeen April 22, 2019, 9:26 p.m. UTC | #1 
On 4/22/19 10:45 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> 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@oracle.com>
> ---
>  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?

>  }
>  
>  static void
>
Darrick J. Wong April 22, 2019, 9:35 p.m. UTC | #2 
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@oracle.com>
> > 
> > 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@oracle.com>
> > ---
> >  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.

--D

> >  }
> >  
> >  static void
> >
Eric Sandeen April 22, 2019, 9:40 p.m. UTC | #3 
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@oracle.com>
>>>
>>> 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@oracle.com>
>>> ---
>>>  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
diff mbox series

Patch

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);
 }
 
 static void