diff mbox

[3/7] fs, xfs: convert xfs_buf_log_item.bli_refcount from atomic_t to refcount_t

Message ID 1487692147-17066-4-git-send-email-elena.reshetova@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Reshetova, Elena Feb. 21, 2017, 3:49 p.m. UTC 
refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 fs/xfs/xfs_buf_item.c  | 16 ++++++++--------
 fs/xfs/xfs_buf_item.h  |  4 +++-
 fs/xfs/xfs_trace.h     |  2 +-
 fs/xfs/xfs_trans_buf.c | 32 ++++++++++++++++----------------
 4 files changed, 28 insertions(+), 26 deletions(-)

Comments

Peter Zijlstra Feb. 21, 2017, 3:59 p.m. UTC | #1 
On Tue, Feb 21, 2017 at 05:49:03PM +0200, Elena Reshetova wrote:
> refcount_t type and corresponding API should be
> used instead of atomic_t when the variable is used as
> a reference counter. This allows to avoid accidental
> refcounter overflows that might lead to use-after-free
> situations.

Changelog forgets to mention if this was runtime tested..


> @@ -371,7 +371,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
>  	ASSERT(bip->bli_item.li_type == XFS_LI_BUF);
>  	ASSERT(!(bip->bli_flags & XFS_BLI_STALE));
>  	ASSERT(!(bip->__bli_format.blf_flags & XFS_BLF_CANCEL));
> -	ASSERT(atomic_read(&bip->bli_refcount) > 0);
> +	ASSERT(refcount_read(&bip->bli_refcount) > 0);
>  
>  	trace_xfs_trans_brelse(bip);
>  
> @@ -419,7 +419,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
>  	/*
>  	 * Drop our reference to the buf log item.
>  	 */
> -	atomic_dec(&bip->bli_refcount);
> +	refcount_dec(&bip->bli_refcount);
>  
>  	/*
>  	 * If the buf item is not tracking data in the log, then
> @@ -432,7 +432,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
>  /***
>  		ASSERT(bp->b_pincount == 0);
>  ***/
> -		ASSERT(atomic_read(&bip->bli_refcount) == 0);
> +		ASSERT(refcount_read(&bip->bli_refcount) == 0);
>  		ASSERT(!(bip->bli_item.li_flags & XFS_LI_IN_AIL));
>  		ASSERT(!(bip->bli_flags & XFS_BLI_INODE_ALLOC_BUF));
>  		xfs_buf_item_relse(bp);


This for example looks dodgy.

That seems to suggest the atomic_dec() there can actually hit 0, which
_will_ generate a WARN.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reshetova, Elena Feb. 21, 2017, 4:06 p.m. UTC | #2 
> On Tue, Feb 21, 2017 at 05:49:03PM +0200, Elena Reshetova wrote:
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> 
> Changelog forgets to mention if this was runtime tested..

It was boot-tested in the whole refcount_t changes pile, which is not very useful for fs anyway. 
What's why we are sending this through maintainers to get through their tests. 
I am sure that testing would be better than what we can do. 

> 
> 
> > @@ -371,7 +371,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> >  	ASSERT(bip->bli_item.li_type == XFS_LI_BUF);
> >  	ASSERT(!(bip->bli_flags & XFS_BLI_STALE));
> >  	ASSERT(!(bip->__bli_format.blf_flags & XFS_BLF_CANCEL));
> > -	ASSERT(atomic_read(&bip->bli_refcount) > 0);
> > +	ASSERT(refcount_read(&bip->bli_refcount) > 0);
> >
> >  	trace_xfs_trans_brelse(bip);
> >
> > @@ -419,7 +419,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> >  	/*
> >  	 * Drop our reference to the buf log item.
> >  	 */
> > -	atomic_dec(&bip->bli_refcount);
> > +	refcount_dec(&bip->bli_refcount);
> >
> >  	/*
> >  	 * If the buf item is not tracking data in the log, then
> > @@ -432,7 +432,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> >  /***
> >  		ASSERT(bp->b_pincount == 0);
> >  ***/
> > -		ASSERT(atomic_read(&bip->bli_refcount) == 0);
> > +		ASSERT(refcount_read(&bip->bli_refcount) == 0);
> >  		ASSERT(!(bip->bli_item.li_flags & XFS_LI_IN_AIL));
> >  		ASSERT(!(bip->bli_flags &
> XFS_BLI_INODE_ALLOC_BUF));
> >  		xfs_buf_item_relse(bp);
> 
> 
> This for example looks dodgy.
> 
> That seems to suggest the atomic_dec() there can actually hit 0, which
> _will_ generate a WARN.

True, but in some of this cases WARN might be ok, I think? As soon as functionality is not changed and object is not reused (by doing refcount_inc on it) anywhere later on. 

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Zijlstra Feb. 21, 2017, 4:27 p.m. UTC | #3 
On Tue, Feb 21, 2017 at 04:06:30PM +0000, Reshetova, Elena wrote:
> > On Tue, Feb 21, 2017 at 05:49:03PM +0200, Elena Reshetova wrote:
> > > refcount_t type and corresponding API should be
> > > used instead of atomic_t when the variable is used as
> > > a reference counter. This allows to avoid accidental
> > > refcounter overflows that might lead to use-after-free
> > > situations.
> > 
> > Changelog forgets to mention if this was runtime tested..
> 
> It was boot-tested in the whole refcount_t changes pile, which is not very useful for fs anyway. 
> What's why we are sending this through maintainers to get through their tests. 
> I am sure that testing would be better than what we can do. 

For FS changes, I would recommend at the very least running xfstests on
an instance (which unlike the name suggests is good for most block
filesystems Linux has).

> > > @@ -432,7 +432,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> > >  /***
> > >  		ASSERT(bp->b_pincount == 0);
> > >  ***/
> > > -		ASSERT(atomic_read(&bip->bli_refcount) == 0);
> > > +		ASSERT(refcount_read(&bip->bli_refcount) == 0);
> > >  		ASSERT(!(bip->bli_item.li_flags & XFS_LI_IN_AIL));
> > >  		ASSERT(!(bip->bli_flags & XFS_BLI_INODE_ALLOC_BUF));
> > >  		xfs_buf_item_relse(bp);
> > 
> > 
> > This for example looks dodgy.
> > 
> > That seems to suggest the atomic_dec() there can actually hit 0, which
> > _will_ generate a WARN.
> 
> True, but in some of this cases WARN might be ok, I think? As soon as
> functionality is not changed and object is not reused (by doing
> refcount_inc on it) anywhere later on. 

No, your conversion should not generate spurious WARN()s.

And also no, atomic_dec() must not hit 0. If you've been _that_ careful
with your reference counting and you absolutely _know_ this is the very
last one, write something like:

  WARN_ON(!refcount_dec_if_one());


Please, stop sending out conversions that haven't been tested. And take
the time to actually look at your own patches. If I can spot fail just
looking through them, so can you (or any of the other many people in
your SoB chain).

Take a little more time and a little extra care.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Zijlstra Feb. 21, 2017, 4:32 p.m. UTC | #4 
On Tue, Feb 21, 2017 at 05:27:58PM +0100, Peter Zijlstra wrote:
> > True, but in some of this cases WARN might be ok, I think? As soon as
> > functionality is not changed and object is not reused (by doing
> > refcount_inc on it) anywhere later on. 
> 
> No, your conversion should not generate spurious WARN()s.
> 
> And also no, atomic_dec() must not hit 0. If you've been _that_ careful

refcount_dec() obviously, atomic_dec() doesn't care one way or the
other.

> with your reference counting and you absolutely _know_ this is the very
> last one, write something like:
> 
>   WARN_ON(!refcount_dec_if_one());
> 
> 
> Please, stop sending out conversions that haven't been tested. And take
> the time to actually look at your own patches. If I can spot fail just
> looking through them, so can you (or any of the other many people in
> your SoB chain).
> 
> Take a little more time and a little extra care.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darrick J. Wong Feb. 21, 2017, 5:06 p.m. UTC | #5 
On Tue, Feb 21, 2017 at 04:06:30PM +0000, Reshetova, Elena wrote:
> > On Tue, Feb 21, 2017 at 05:49:03PM +0200, Elena Reshetova wrote:
> > > refcount_t type and corresponding API should be
> > > used instead of atomic_t when the variable is used as
> > > a reference counter. This allows to avoid accidental
> > > refcounter overflows that might lead to use-after-free
> > > situations.
> > 
> > Changelog forgets to mention if this was runtime tested..
> 
> It was boot-tested in the whole refcount_t changes pile, which is not very useful for fs anyway. 
> What's why we are sending this through maintainers to get through their tests. 
> I am sure that testing would be better than what we can do. 

If you're going to go around making this many changes to XFS (or any
other filesystem), please run the changes through xfstests first.
Many fs projects (not just XFS) record their test cases there.

I think the kernel 0day build service is supposed to do that
automatically...

--D

> 
> > 
> > 
> > > @@ -371,7 +371,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> > >  	ASSERT(bip->bli_item.li_type == XFS_LI_BUF);
> > >  	ASSERT(!(bip->bli_flags & XFS_BLI_STALE));
> > >  	ASSERT(!(bip->__bli_format.blf_flags & XFS_BLF_CANCEL));
> > > -	ASSERT(atomic_read(&bip->bli_refcount) > 0);
> > > +	ASSERT(refcount_read(&bip->bli_refcount) > 0);
> > >
> > >  	trace_xfs_trans_brelse(bip);
> > >
> > > @@ -419,7 +419,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> > >  	/*
> > >  	 * Drop our reference to the buf log item.
> > >  	 */
> > > -	atomic_dec(&bip->bli_refcount);
> > > +	refcount_dec(&bip->bli_refcount);
> > >
> > >  	/*
> > >  	 * If the buf item is not tracking data in the log, then
> > > @@ -432,7 +432,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> > >  /***
> > >  		ASSERT(bp->b_pincount == 0);
> > >  ***/
> > > -		ASSERT(atomic_read(&bip->bli_refcount) == 0);
> > > +		ASSERT(refcount_read(&bip->bli_refcount) == 0);
> > >  		ASSERT(!(bip->bli_item.li_flags & XFS_LI_IN_AIL));
> > >  		ASSERT(!(bip->bli_flags &
> > XFS_BLI_INODE_ALLOC_BUF));
> > >  		xfs_buf_item_relse(bp);
> > 
> > 
> > This for example looks dodgy.
> > 
> > That seems to suggest the atomic_dec() there can actually hit 0, which
> > _will_ generate a WARN.
> 
> True, but in some of this cases WARN might be ok, I think? As soon as functionality is not changed and object is not reused (by doing refcount_inc on it) anywhere later on. 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Brian Foster Feb. 21, 2017, 7:25 p.m. UTC | #6 
On Tue, Feb 21, 2017 at 09:06:20AM -0800, Darrick J. Wong wrote:
> On Tue, Feb 21, 2017 at 04:06:30PM +0000, Reshetova, Elena wrote:
> > > On Tue, Feb 21, 2017 at 05:49:03PM +0200, Elena Reshetova wrote:
> > > > refcount_t type and corresponding API should be
> > > > used instead of atomic_t when the variable is used as
> > > > a reference counter. This allows to avoid accidental
> > > > refcounter overflows that might lead to use-after-free
> > > > situations.
> > > 
> > > Changelog forgets to mention if this was runtime tested..
> > 
> > It was boot-tested in the whole refcount_t changes pile, which is not very useful for fs anyway. 
> > What's why we are sending this through maintainers to get through their tests. 
> > I am sure that testing would be better than what we can do. 
> 
> If you're going to go around making this many changes to XFS (or any
> other filesystem), please run the changes through xfstests first.
> Many fs projects (not just XFS) record their test cases there.
> 
> I think the kernel 0day build service is supposed to do that
> automatically...
> 

Be sure to use CONFIG_XFS_DEBUG and/or CONFIG_XFS_WARN to capture any
potential assert failures as well.

Brian

> --D
> 
> > 
> > > 
> > > 
> > > > @@ -371,7 +371,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> > > >  	ASSERT(bip->bli_item.li_type == XFS_LI_BUF);
> > > >  	ASSERT(!(bip->bli_flags & XFS_BLI_STALE));
> > > >  	ASSERT(!(bip->__bli_format.blf_flags & XFS_BLF_CANCEL));
> > > > -	ASSERT(atomic_read(&bip->bli_refcount) > 0);
> > > > +	ASSERT(refcount_read(&bip->bli_refcount) > 0);
> > > >
> > > >  	trace_xfs_trans_brelse(bip);
> > > >
> > > > @@ -419,7 +419,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> > > >  	/*
> > > >  	 * Drop our reference to the buf log item.
> > > >  	 */
> > > > -	atomic_dec(&bip->bli_refcount);
> > > > +	refcount_dec(&bip->bli_refcount);
> > > >
> > > >  	/*
> > > >  	 * If the buf item is not tracking data in the log, then
> > > > @@ -432,7 +432,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> > > >  /***
> > > >  		ASSERT(bp->b_pincount == 0);
> > > >  ***/
> > > > -		ASSERT(atomic_read(&bip->bli_refcount) == 0);
> > > > +		ASSERT(refcount_read(&bip->bli_refcount) == 0);
> > > >  		ASSERT(!(bip->bli_item.li_flags & XFS_LI_IN_AIL));
> > > >  		ASSERT(!(bip->bli_flags &
> > > XFS_BLI_INODE_ALLOC_BUF));
> > > >  		xfs_buf_item_relse(bp);
> > > 
> > > 
> > > This for example looks dodgy.
> > > 
> > > That seems to suggest the atomic_dec() there can actually hit 0, which
> > > _will_ generate a WARN.
> > 
> > True, but in some of this cases WARN might be ok, I think? As soon as functionality is not changed and object is not reused (by doing refcount_inc on it) anywhere later on. 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reshetova, Elena Feb. 22, 2017, 11:26 a.m. UTC | #7 
> On Tue, Feb 21, 2017 at 09:06:20AM -0800, Darrick J. Wong wrote:
> > On Tue, Feb 21, 2017 at 04:06:30PM +0000, Reshetova, Elena wrote:
> > > > On Tue, Feb 21, 2017 at 05:49:03PM +0200, Elena Reshetova wrote:
> > > > > refcount_t type and corresponding API should be
> > > > > used instead of atomic_t when the variable is used as
> > > > > a reference counter. This allows to avoid accidental
> > > > > refcounter overflows that might lead to use-after-free
> > > > > situations.
> > > >
> > > > Changelog forgets to mention if this was runtime tested..
> > >
> > > It was boot-tested in the whole refcount_t changes pile, which is not very
> useful for fs anyway.
> > > What's why we are sending this through maintainers to get through their
> tests.
> > > I am sure that testing would be better than what we can do.
> >
> > If you're going to go around making this many changes to XFS (or any
> > other filesystem), please run the changes through xfstests first.
> > Many fs projects (not just XFS) record their test cases there.
> >
> > I think the kernel 0day build service is supposed to do that
> > automatically...
> >
> 
> Be sure to use CONFIG_XFS_DEBUG and/or CONFIG_XFS_WARN to capture
> any
> potential assert failures as well.

Thanks for pointing to this! I have been actually asking before on how to runtime test things more with all our patches before submission, but got reply around: "submit to maintainers, they know how to do it". 
I think we need to look into 0day automated testing, otherwise since we make changes to many FSes, testing this manually would be little fun...

Best Regards,
Elena
> 
> Brian
> 
> > --D
> >
> > >
> > > >
> > > >
> > > > > @@ -371,7 +371,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> > > > >  	ASSERT(bip->bli_item.li_type == XFS_LI_BUF);
> > > > >  	ASSERT(!(bip->bli_flags & XFS_BLI_STALE));
> > > > >  	ASSERT(!(bip->__bli_format.blf_flags & XFS_BLF_CANCEL));
> > > > > -	ASSERT(atomic_read(&bip->bli_refcount) > 0);
> > > > > +	ASSERT(refcount_read(&bip->bli_refcount) > 0);
> > > > >
> > > > >  	trace_xfs_trans_brelse(bip);
> > > > >
> > > > > @@ -419,7 +419,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> > > > >  	/*
> > > > >  	 * Drop our reference to the buf log item.
> > > > >  	 */
> > > > > -	atomic_dec(&bip->bli_refcount);
> > > > > +	refcount_dec(&bip->bli_refcount);
> > > > >
> > > > >  	/*
> > > > >  	 * If the buf item is not tracking data in the log, then
> > > > > @@ -432,7 +432,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> > > > >  /***
> > > > >  		ASSERT(bp->b_pincount == 0);
> > > > >  ***/
> > > > > -		ASSERT(atomic_read(&bip->bli_refcount) == 0);
> > > > > +		ASSERT(refcount_read(&bip->bli_refcount) == 0);
> > > > >  		ASSERT(!(bip->bli_item.li_flags & XFS_LI_IN_AIL));
> > > > >  		ASSERT(!(bip->bli_flags &
> > > > XFS_BLI_INODE_ALLOC_BUF));
> > > > >  		xfs_buf_item_relse(bp);
> > > >
> > > >
> > > > This for example looks dodgy.
> > > >
> > > > That seems to suggest the atomic_dec() there can actually hit 0, which
> > > > _will_ generate a WARN.
> > >
> > > True, but in some of this cases WARN might be ok, I think? As soon as
> functionality is not changed and object is not reused (by doing refcount_inc on
> it) anywhere later on.
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 0306168..f6063ad 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -137,7 +137,7 @@  xfs_buf_item_size(
 	struct xfs_buf_log_item	*bip = BUF_ITEM(lip);
 	int			i;
 
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 	if (bip->bli_flags & XFS_BLI_STALE) {
 		/*
 		 * The buffer is stale, so all we need to log
@@ -316,7 +316,7 @@  xfs_buf_item_format(
 	uint			offset = 0;
 	int			i;
 
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 	ASSERT((bip->bli_flags & XFS_BLI_LOGGED) ||
 	       (bip->bli_flags & XFS_BLI_STALE));
 	ASSERT((bip->bli_flags & XFS_BLI_STALE) ||
@@ -383,14 +383,14 @@  xfs_buf_item_pin(
 {
 	struct xfs_buf_log_item	*bip = BUF_ITEM(lip);
 
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 	ASSERT((bip->bli_flags & XFS_BLI_LOGGED) ||
 	       (bip->bli_flags & XFS_BLI_ORDERED) ||
 	       (bip->bli_flags & XFS_BLI_STALE));
 
 	trace_xfs_buf_item_pin(bip);
 
-	atomic_inc(&bip->bli_refcount);
+	refcount_inc(&bip->bli_refcount);
 	atomic_inc(&bip->bli_buf->b_pin_count);
 }
 
@@ -419,11 +419,11 @@  xfs_buf_item_unpin(
 	int		freed;
 
 	ASSERT(bp->b_fspriv == bip);
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 
 	trace_xfs_buf_item_unpin(bip);
 
-	freed = atomic_dec_and_test(&bip->bli_refcount);
+	freed = refcount_dec_and_test(&bip->bli_refcount);
 
 	if (atomic_dec_and_test(&bp->b_pin_count))
 		wake_up_all(&bp->b_waiters);
@@ -605,7 +605,7 @@  xfs_buf_item_unlock(
 		trace_xfs_buf_item_unlock_stale(bip);
 		ASSERT(bip->__bli_format.blf_flags & XFS_BLF_CANCEL);
 		if (!aborted) {
-			atomic_dec(&bip->bli_refcount);
+			refcount_dec(&bip->bli_refcount);
 			return;
 		}
 	}
@@ -642,7 +642,7 @@  xfs_buf_item_unlock(
 	 * it in this case, because an aborted transaction has already shut the
 	 * filesystem down and this is the last chance we will have to do so.
 	 */
-	if (atomic_dec_and_test(&bip->bli_refcount)) {
+	if (refcount_dec_and_test(&bip->bli_refcount)) {
 		if (clean)
 			xfs_buf_item_relse(bp);
 		else if (aborted) {
diff --git a/fs/xfs/xfs_buf_item.h b/fs/xfs/xfs_buf_item.h
index f7eba99..7bbdef7 100644
--- a/fs/xfs/xfs_buf_item.h
+++ b/fs/xfs/xfs_buf_item.h
@@ -18,6 +18,8 @@ 
 #ifndef	__XFS_BUF_ITEM_H__
 #define	__XFS_BUF_ITEM_H__
 
+#include <linux/refcount.h>
+
 /* kernel only definitions */
 
 /* buf log item flags */
@@ -55,7 +57,7 @@  typedef struct xfs_buf_log_item {
 	struct xfs_buf		*bli_buf;	/* real buffer pointer */
 	unsigned int		bli_flags;	/* misc flags */
 	unsigned int		bli_recur;	/* lock recursion count */
-	atomic_t		bli_refcount;	/* cnt of tp refs */
+	refcount_t		bli_refcount;	/* cnt of tp refs */
 	int			bli_format_count;	/* count of headers */
 	struct xfs_buf_log_format *bli_formats;	/* array of in-log header ptrs */
 	struct xfs_buf_log_format __bli_format;	/* embedded in-log header */
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 8fc98d5..4afd160 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -479,7 +479,7 @@  DECLARE_EVENT_CLASS(xfs_buf_item_class,
 		__entry->dev = bip->bli_buf->b_target->bt_dev;
 		__entry->bli_flags = bip->bli_flags;
 		__entry->bli_recur = bip->bli_recur;
-		__entry->bli_refcount = atomic_read(&bip->bli_refcount);
+		__entry->bli_refcount = refcount_read(&bip->bli_refcount);
 		__entry->buf_bno = bip->bli_buf->b_bn;
 		__entry->buf_len = BBTOB(bip->bli_buf->b_length);
 		__entry->buf_flags = bip->bli_buf->b_flags;
diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
index 8ee29ca..fa7f213 100644
--- a/fs/xfs/xfs_trans_buf.c
+++ b/fs/xfs/xfs_trans_buf.c
@@ -97,7 +97,7 @@  _xfs_trans_bjoin(
 	/*
 	 * Take a reference for this transaction on the buf item.
 	 */
-	atomic_inc(&bip->bli_refcount);
+	refcount_inc(&bip->bli_refcount);
 
 	/*
 	 * Get a log_item_desc to point at the new item.
@@ -161,7 +161,7 @@  xfs_trans_get_buf_map(
 		ASSERT(bp->b_transp == tp);
 		bip = bp->b_fspriv;
 		ASSERT(bip != NULL);
-		ASSERT(atomic_read(&bip->bli_refcount) > 0);
+		ASSERT(refcount_read(&bip->bli_refcount) > 0);
 		bip->bli_recur++;
 		trace_xfs_trans_get_buf_recur(bip);
 		return bp;
@@ -212,7 +212,7 @@  xfs_trans_getsb(xfs_trans_t	*tp,
 	if (bp->b_transp == tp) {
 		bip = bp->b_fspriv;
 		ASSERT(bip != NULL);
-		ASSERT(atomic_read(&bip->bli_refcount) > 0);
+		ASSERT(refcount_read(&bip->bli_refcount) > 0);
 		bip->bli_recur++;
 		trace_xfs_trans_getsb_recur(bip);
 		return bp;
@@ -282,7 +282,7 @@  xfs_trans_read_buf_map(
 		bip = bp->b_fspriv;
 		bip->bli_recur++;
 
-		ASSERT(atomic_read(&bip->bli_refcount) > 0);
+		ASSERT(refcount_read(&bip->bli_refcount) > 0);
 		trace_xfs_trans_read_buf_recur(bip);
 		*bpp = bp;
 		return 0;
@@ -371,7 +371,7 @@  xfs_trans_brelse(xfs_trans_t	*tp,
 	ASSERT(bip->bli_item.li_type == XFS_LI_BUF);
 	ASSERT(!(bip->bli_flags & XFS_BLI_STALE));
 	ASSERT(!(bip->__bli_format.blf_flags & XFS_BLF_CANCEL));
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 
 	trace_xfs_trans_brelse(bip);
 
@@ -419,7 +419,7 @@  xfs_trans_brelse(xfs_trans_t	*tp,
 	/*
 	 * Drop our reference to the buf log item.
 	 */
-	atomic_dec(&bip->bli_refcount);
+	refcount_dec(&bip->bli_refcount);
 
 	/*
 	 * If the buf item is not tracking data in the log, then
@@ -432,7 +432,7 @@  xfs_trans_brelse(xfs_trans_t	*tp,
 /***
 		ASSERT(bp->b_pincount == 0);
 ***/
-		ASSERT(atomic_read(&bip->bli_refcount) == 0);
+		ASSERT(refcount_read(&bip->bli_refcount) == 0);
 		ASSERT(!(bip->bli_item.li_flags & XFS_LI_IN_AIL));
 		ASSERT(!(bip->bli_flags & XFS_BLI_INODE_ALLOC_BUF));
 		xfs_buf_item_relse(bp);
@@ -458,7 +458,7 @@  xfs_trans_bhold(xfs_trans_t	*tp,
 	ASSERT(bip != NULL);
 	ASSERT(!(bip->bli_flags & XFS_BLI_STALE));
 	ASSERT(!(bip->__bli_format.blf_flags & XFS_BLF_CANCEL));
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 
 	bip->bli_flags |= XFS_BLI_HOLD;
 	trace_xfs_trans_bhold(bip);
@@ -478,7 +478,7 @@  xfs_trans_bhold_release(xfs_trans_t	*tp,
 	ASSERT(bip != NULL);
 	ASSERT(!(bip->bli_flags & XFS_BLI_STALE));
 	ASSERT(!(bip->__bli_format.blf_flags & XFS_BLF_CANCEL));
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 	ASSERT(bip->bli_flags & XFS_BLI_HOLD);
 
 	bip->bli_flags &= ~XFS_BLI_HOLD;
@@ -520,7 +520,7 @@  xfs_trans_log_buf(xfs_trans_t	*tp,
 	 */
 	bp->b_flags |= XBF_DONE;
 
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 	bp->b_iodone = xfs_buf_iodone_callbacks;
 	bip->bli_item.li_cb = xfs_buf_iodone;
 
@@ -591,7 +591,7 @@  xfs_trans_binval(
 
 	ASSERT(bp->b_transp == tp);
 	ASSERT(bip != NULL);
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 
 	trace_xfs_trans_binval(bip);
 
@@ -645,7 +645,7 @@  xfs_trans_inode_buf(
 
 	ASSERT(bp->b_transp == tp);
 	ASSERT(bip != NULL);
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 
 	bip->bli_flags |= XFS_BLI_INODE_BUF;
 	xfs_trans_buf_set_type(tp, bp, XFS_BLFT_DINO_BUF);
@@ -669,7 +669,7 @@  xfs_trans_stale_inode_buf(
 
 	ASSERT(bp->b_transp == tp);
 	ASSERT(bip != NULL);
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 
 	bip->bli_flags |= XFS_BLI_STALE_INODE;
 	bip->bli_item.li_cb = xfs_buf_iodone;
@@ -694,7 +694,7 @@  xfs_trans_inode_alloc_buf(
 
 	ASSERT(bp->b_transp == tp);
 	ASSERT(bip != NULL);
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 
 	bip->bli_flags |= XFS_BLI_INODE_ALLOC_BUF;
 	xfs_trans_buf_set_type(tp, bp, XFS_BLFT_DINO_BUF);
@@ -717,7 +717,7 @@  xfs_trans_ordered_buf(
 
 	ASSERT(bp->b_transp == tp);
 	ASSERT(bip != NULL);
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 
 	bip->bli_flags |= XFS_BLI_ORDERED;
 	trace_xfs_buf_item_ordered(bip);
@@ -740,7 +740,7 @@  xfs_trans_buf_set_type(
 
 	ASSERT(bp->b_transp == tp);
 	ASSERT(bip != NULL);
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 
 	xfs_blft_to_flags(&bip->__bli_format, type);
 }