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