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