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