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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxx
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
