Re: [PATCH 10/13] xfs: provide a centralized method for verifying inline fork data

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

On Tue, Dec 19, 2017 at 05:06:40PM +1100, Dave Chinner wrote:
> On Wed, Dec 13, 2017 at 03:59:13PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Replace the current haphazard dir2 shortform verifier callsites with a
> > centralized verifier function that can be called either with the default
> > verifier functions or with a custom set.  This helps us strengthen
> > integrity checking while providing us with flexibility for repair tools.
> > 
> > xfs_repair wants this to be able to supply its own verifier functions
> > when trying to fix possibly corrupt metadata.
> 
> I was about to say "this looks a little over-engineered" then I read
> the commit message.... :P
> 
> Ok, so it pulls the fork verication out of the libxfs inode read code,
> and instead slots it into the kernel inode cache miss path just
> after the inode has been read. Same effect - newly read inodes get
> their forks verified before use. ANd they same thing happens in
> xfs_iflush_int(), which is private to the kernel anyway....

<nod> It's a little over-engineered for the kernel; the real point of
all this is to check local format forks by default while enabling
xfs_repair to skip those checks if, say, it wants to fix an inode with
broken xattrs by zapping the attr fork.

> > +
> > +/* Default fork content verifiers. */
> > +struct xfs_ifork_ops xfs_default_ifork_ops = {
> > +	.verify_attr	= xfs_attr_shortform_verify,
> > +	.verify_dir	= xfs_dir2_sf_verify,
> > +	.verify_symlink	= xfs_symlink_shortform_verify,
> > +};
> > +
> > +/* Verify the inline contents of the data fork of an inode. */
> > +xfs_failaddr_t
> > +xfs_ifork_verify_data(
> > +	struct xfs_inode	*ip,
> > +	struct xfs_ifork_ops	*ops)
> > +{
> > +	xfs_ifork_verifier_t	fn = NULL;
> > +
> > +	/* Non-local data fork, we're done. */
> > +	if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL)
> > +		return NULL;
> > +
> > +	/* Check the inline data fork if there is one. */
> > +	if (S_ISDIR(VFS_I(ip)->i_mode))
> > +		fn = ops->verify_dir;
> > +	else if (S_ISLNK(VFS_I(ip)->i_mode))
> > +		fn = ops->verify_symlink;
> > +
> > +	if (fn)
> > +		return fn(ip);
> > +	return NULL;
> > +}
> 
> Seems a little convoluted, especially if we grow more cases. This
> seems simpler already:
> 
> 	switch(VFS_I(ip)->i_mode & S_IFMT) {
> 	case S_IFDIR:
> 		return ops->verify_dir(ip);
> 	case S_IFLNK:
> 		return ops->verify_symlink(ip);
> 	default:
> 		break;
> 	}
> 	return NULL;

Fixed.

> > +
> > +/* Verify the inline contents of the attr fork of an inode. */
> > +xfs_failaddr_t
> > +xfs_ifork_verify_attr(
> > +	struct xfs_inode	*ip,
> > +	struct xfs_ifork_ops	*ops)
> > +{
> > +	xfs_ifork_verifier_t	fn = NULL;
> > +
> > +	/* There has to be an attr fork allocated if aformat is local. */
> > +	if (ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
> > +		if (XFS_IFORK_PTR(ip, XFS_ATTR_FORK))
> > +			fn = ops->verify_attr;
> > +		else
> > +			return __this_address;
> > +	}
> > +
> > +	if (fn)
> > +		return fn(ip);
> > +	return NULL;
> 
> And this could be stright lined as:
> 
> 	if (ip->i_d.di_aformat != XFS_DINODE_FMT_LOCAL)
> 		return NULL;
> 	if (!XFS_IFORK_PTR(ip, XFS_ATTR_FORK))
> 		return __this_address;
> 
> 	return ops->verify_attr(ip);

Fixed.

--D

> -Dave.
> 
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> 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