Re: [PATCH 5/5] fs/xfs: Allow toggle of physical DAX flag

[Dave Chinner]

On Mon, Oct 21, 2019 at 03:49:31PM -0700, Ira Weiny wrote:
> On Mon, Oct 21, 2019 at 11:45:36AM +1100, Dave Chinner wrote:
> > On Sun, Oct 20, 2019 at 08:59:35AM -0700, ira.weiny@xxxxxxxxx wrote:
> > > @@ -1232,12 +1233,10 @@ xfs_diflags_to_linux(
> > >  		inode->i_flags |= S_NOATIME;
> > >  	else
> > >  		inode->i_flags &= ~S_NOATIME;
> > > -#if 0	/* disabled until the flag switching races are sorted out */
> > >  	if (xflags & FS_XFLAG_DAX)
> > >  		inode->i_flags |= S_DAX;
> > >  	else
> > >  		inode->i_flags &= ~S_DAX;
> > > -#endif
> > 
> > This code has bit-rotted. See xfs_setup_iops(), where we now have a
> > different inode->i_mapping->a_ops for DAX inodes.
> 
> :-(
> 
> > 
> > That, fundamentally, is the issue here - it's not setting/clearing
> > the DAX flag that is the issue, it's doing a swap of the
> > mapping->a_ops while there may be other code using that ops
> > structure.
> > 
> > IOWs, if there is any code anywhere in the kernel that
> > calls an address space op without holding one of the three locks we
> > hold here (i_rwsem, MMAPLOCK, ILOCK) then it can race with the swap
> > of the address space operations.
> > 
> > By limiting the address space swap to file sizes of zero, we rule
> > out the page fault path (mmap of a zero length file segv's with an
> > access beyond EOF on the first read/write page fault, right?).
> 
> Yes I checked that and thought we were safe here...
> 
> > However, other aops callers that might run unlocked and do the wrong
> > thing if the aops pointer is swapped between check of the aop method
> > existing and actually calling it even if the file size is zero?
> > 
> > A quick look shows that FIBMAP (ioctl_fibmap())) looks susceptible
> > to such a race condition with the current definitions of the XFS DAX
> > aops. I'm guessing there will be others, but I haven't looked
> > further than this...
> 
> I'll check for others and think on what to do about this.  ext4 will have the
> same problem I think.  :-(
> 
> I don't suppose using a single a_ops for both DAX and non-DAX is palatable?

IMO, no. It means we have to check IS_DAX() in every aops,
and replicate a bunch of callouts to generic code. i.e this sort of
thing:

	if (aops->method)
		return aops->method(...)

	/* do something else */

results in us having to replicate that logic as something like:

	if (!IS_DAX)
		return filesystem_aops_method()

	/* do something else */

Indeed, the calling code may well do the wrong thing if we have
methods defined just to add IS_DAX() checks to avoid using that
functionality because the caller now thinks that functionality is
supported when in fact it isn't.

So it seems to me like an even bigger can of worms to try to use a
single aops structure for vastly different functionality....

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx