Re: [PATCH 2/4] xfs: don't propagate invalid extent size hints to new files

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, May 14, 2021 at 08:38:27AM -0400, Brian Foster wrote:
> On Wed, May 12, 2021 at 06:01:53PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Under the current inode extent size hint validation rules, it's possible
> > to set extent size hints on directories along with an 'inherit' flag so
> > that the values will be propagated to newly created regular files.  (The
> > directories themselves do not care about the hint values.)
> > 
> > For these directories, the alignment of the hint is checked against the
> > data device even if the directory also has the rtinherit hint set, which
> > means that one can set a directory's hint value to something that isn't
> > an integer multiple of the realtime extent size.  This isn't a problem
> > for the directory itself, but the validation routines require rt extent
> > alignment for realtime files.
> > 
> > If the unaligned hint value and the realtime bit are both propagated
> > into a newly created regular realtime file, we end up writing out an
> > incorrect hint that trips the verifiers the next time we try to read the
> > inode buffer, and the fs shuts down.  Fix this by cancelling the hint
> > propagation if it would cause problems.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> 
> Hmm.. this seems a bit unfortunate. Is the purpose of this flag
> cancellation behavior basically to accommodate existing filesystems that
> might have this incompatible combination in place?

Yes.  The incompatible combination when set on a directory is benign,
but setting it on regular files gets us into real trouble, so the goal
here is to end the propagation of the incompatible values.

--D

> Brian
> 
> >  fs/xfs/xfs_inode.c |   19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > 
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index 0369eb22c1bb..db81e8c22708 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -689,6 +689,7 @@ xfs_inode_inherit_flags(
> >  	struct xfs_inode	*ip,
> >  	const struct xfs_inode	*pip)
> >  {
> > +	xfs_failaddr_t		failaddr;
> >  	unsigned int		di_flags = 0;
> >  	umode_t			mode = VFS_I(ip)->i_mode;
> >  
> > @@ -728,6 +729,14 @@ xfs_inode_inherit_flags(
> >  	if (pip->i_diflags & XFS_DIFLAG_FILESTREAM)
> >  		di_flags |= XFS_DIFLAG_FILESTREAM;
> >  
> > +	/* Make sure the extsize actually validates properly. */
> > +	failaddr = xfs_inode_validate_extsize(ip->i_mount, ip->i_extsize,
> > +			VFS_I(ip)->i_mode, ip->i_diflags);
> > +	if (failaddr) {
> > +		di_flags &= ~(XFS_DIFLAG_EXTSIZE | XFS_DIFLAG_EXTSZINHERIT);
> > +		ip->i_extsize = 0;
> > +	}
> > +
> >  	ip->i_diflags |= di_flags;
> >  }
> >  
> > @@ -737,12 +746,22 @@ xfs_inode_inherit_flags2(
> >  	struct xfs_inode	*ip,
> >  	const struct xfs_inode	*pip)
> >  {
> > +	xfs_failaddr_t		failaddr;
> > +
> >  	if (pip->i_diflags2 & XFS_DIFLAG2_COWEXTSIZE) {
> >  		ip->i_diflags2 |= XFS_DIFLAG2_COWEXTSIZE;
> >  		ip->i_cowextsize = pip->i_cowextsize;
> >  	}
> >  	if (pip->i_diflags2 & XFS_DIFLAG2_DAX)
> >  		ip->i_diflags2 |= XFS_DIFLAG2_DAX;
> > +
> > +	/* Make sure the cowextsize actually validates properly. */
> > +	failaddr = xfs_inode_validate_cowextsize(ip->i_mount, ip->i_cowextsize,
> > +			VFS_I(ip)->i_mode, ip->i_diflags, ip->i_diflags2);
> > +	if (failaddr) {
> > +		ip->i_diflags2 &= ~XFS_DIFLAG2_COWEXTSIZE;
> > +		ip->i_cowextsize = 0;
> > +	}
> >  }
> >  
> >  /*
> > 
>