linux-kernel - Re: [PATCH 3/7] fs, xfs: convert xfs_buf_log_item.bli_refcount from atomic_t to refcount

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170221170620.GD5846@birch.djwong.org>
Date:   Tue, 21 Feb 2017 09:06:20 -0800
From:   "Darrick J. Wong" <darrick.wong@...cle.com>
To:     "Reshetova, Elena" <elena.reshetova@...el.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-xfs@...r.kernel.org" <linux-xfs@...r.kernel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        Hans Liljestrand <ishkamiel@...il.com>,
        Kees Cook <keescook@...omium.org>,
        David Windsor <dwindsor@...il.com>
Subject: Re: [PATCH 3/7] fs, xfs: convert xfs_buf_log_item.bli_refcount from
 atomic_t to refcount_t

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