Re: [PATCH 1/7] xfs: fix toctou race when locking an inode to access the data map

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Tue, Jan 31, 2017 at 05:26:58AM -0800, Christoph Hellwig wrote:
> On Mon, Jan 30, 2017 at 04:23:10PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > We use di_format and if_flags to decide whether we're grabbing the ilock
> > in btree mode (btree extents not loaded) or shared mode (anything else),
> > but the state of those fields can be changed by other threads that are
> > also trying to load the btree extents -- IFEXTENTS gets set before the
> > _bmap_read_extents call and cleared if it fails.  Therefore, once we've
> > grabbed the shared ilock we have to re-check the fields to see if we
> > actually need to upgrade to the exclusive ilock in order to try loading
> > the extents.
> > 
> > Without this patch, we trigger ilock assert failures when a bunch of
> > threads try to access a btree format directory with a corrupt bmbt root
> > and corrupt the incore data structures, leading to a crash.
> 
> I see the problem here, but I really don't like the fix.  Instead
> I'd much rather check for a new flag that tells that the extent list
> hasn't been read, which can only be cleared under the exclusive
> ilock.  That way we shouldn't need any additional relocking or
> checks.

I'm confused --

I thought XFS_IFEXTENTS means "extents have been read", which is the
inverse of the flag you propose.  AFAICT the bit is only ever set (or
cleared) with ILOCK_EXCL held, so the problem here is that we're
performing an unlocked read of if_flags prior to actually taking the
lock that we need.

On the other hand, I /think/ it's the case that none of the functions
called in _iread_extents actually cares about IFEXTENTS being set, so
perhaps an alternative could be to avoid setting the bit until we've
successfully read in all the bmbt records?

I'll try that out and report back.

--D

> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html