Re: [PATCH 3/4] xfs: validate extsz hints against rt extent size when rtinherit is set

[Brian Foster <bfoster@redhat.com>]

On Fri, May 14, 2021 at 11:22:53AM -0700, Darrick J. Wong wrote:
> On Fri, May 14, 2021 at 08:38:35AM -0400, Brian Foster wrote:
> > On Wed, May 12, 2021 at 06:01:58PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > The RTINHERIT bit can be set on a directory so that newly created
> > > regular files will have the REALTIME bit set to store their data on the
> > > realtime volume.  If an extent size hint (and EXTSZINHERIT) are set on
> > > the directory, the hint will also be copied into the new file.
> > > 
> > > As pointed out in previous patches, for realtime files we require the
> > > extent size hint be an integer multiple of the realtime extent, but we
> > > don't perform the same validation on a directory with both RTINHERIT and
> > > EXTSZINHERIT set, even though the only use-case of that combination is
> > > to propagate extent size hints into new realtime files.  This leads to
> > > inode corruption errors when the bad values are propagated.
> > > 
> > > Strengthen the validation routine to avoid this situation and fix the
> > > open-coded unit conversion while we're at it.  Note that this is
> > > technically a breaking change to the ondisk format, but the risk should
> > > be minimal because (a) most vendors disable realtime, (b) letting
> > > unaligned hints propagate to new files would immediately crash the
> > > filesystem, and (c) xfs_repair flags such filesystems as corrupt, so
> > > anyone with such a configuration is broken already anyway.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > 
> > Ok, so this looks more like a proper fix, but does this turn an existing
> > directory with (rtinherit && extszinherit) and a badly aligned extsz
> > hint into a read validation error?
> 
> Hmm, you're right.  This fix needs to be more targeted in its nature.
> For non-rt filesystems, the rtinherit bit being set on a directory is
> benign because we won't set the realtime bit on new files, so there's no
> need to introduce a new verifier error that will fail existing
> filesystems.
> 
> We /do/ need to trap the misconfiguration for filesystems with an rt
> volume because those filesystems will fail if the propagation happens.
> 
> I think the solution here is to change the verifier check here to
> prevent the spread of bad extent size hints:
> 
> 	if (rt_flag || (xfs_sb_version_hasrealtime(&mp->m_sb) &&
> 			rtinherit_flag && inherit_flag))
> 		blocksize_bytes = XFS_FSB_TO_B(mp, mp->m_sb.sb_rextsize);
> 	else
> 		blocksize_bytes = mp->m_sb.sb_blocksize;
> 
> ...and add a check to xfs_ioctl_setattr_check_extsize to prevent
> sysadmins from misconfiguring directories in the first place.
> 

It definitely makes sense to prevent this misconfiguration going
forward, but I'm a little confused on the intended behavior for
filesystems where this is already present (and not benign). ISTM the
previous patch is intended to allow the filesystem to continue running
with the added behavior that we restrict further propagation of
preexisting misconfigured extent size hints, but would this patch
trigger a verifier failure on read of such a misconfigured directory
inode..?

Brian

> --D
> 
> > Brian
> > 
> > >  fs/xfs/libxfs/xfs_inode_buf.c |    7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> > > index 5c9a7440d9e4..25261dd73290 100644
> > > --- a/fs/xfs/libxfs/xfs_inode_buf.c
> > > +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> > > @@ -569,19 +569,20 @@ xfs_inode_validate_extsize(
> > >  	uint16_t			mode,
> > >  	uint16_t			flags)
> > >  {
> > > -	bool				rt_flag;
> > > +	bool				rt_flag, rtinherit_flag;
> > >  	bool				hint_flag;
> > >  	bool				inherit_flag;
> > >  	uint32_t			extsize_bytes;
> > >  	uint32_t			blocksize_bytes;
> > >  
> > >  	rt_flag = (flags & XFS_DIFLAG_REALTIME);
> > > +	rtinherit_flag = (flags & XFS_DIFLAG_RTINHERIT);
> > >  	hint_flag = (flags & XFS_DIFLAG_EXTSIZE);
> > >  	inherit_flag = (flags & XFS_DIFLAG_EXTSZINHERIT);
> > >  	extsize_bytes = XFS_FSB_TO_B(mp, extsize);
> > >  
> > > -	if (rt_flag)
> > > -		blocksize_bytes = mp->m_sb.sb_rextsize << mp->m_sb.sb_blocklog;
> > > +	if (rt_flag || (rtinherit_flag && inherit_flag))
> > > +		blocksize_bytes = XFS_FSB_TO_B(mp, mp->m_sb.sb_rextsize);
> > >  	else
> > >  		blocksize_bytes = mp->m_sb.sb_blocksize;
> > >  
> > > 
> > 
>