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

[Darrick J. Wong]

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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html