| Message ID | 151320955376.30654.3972078644450529725.stgit@magnolia (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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.... > + > +/* 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; > + > +/* 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); -Dave.
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 -- 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
diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c index a92395a..7756103 100644 --- a/fs/xfs/libxfs/xfs_inode_fork.c +++ b/fs/xfs/libxfs/xfs_inode_fork.c @@ -35,6 +35,8 @@ #include "xfs_da_format.h" #include "xfs_da_btree.h" #include "xfs_dir2_priv.h" +#include "xfs_attr_leaf.h" +#include "xfs_shared.h" kmem_zone_t *xfs_ifork_zone; @@ -97,14 +99,6 @@ xfs_iformat_fork( if (error) return error; - /* Check inline dir contents. */ - if (S_ISDIR(inode->i_mode) && dip->di_format == XFS_DINODE_FMT_LOCAL) { - if (xfs_dir2_sf_verify(ip)) { - xfs_idestroy_fork(ip, XFS_DATA_FORK); - return -EFSCORRUPTED; - } - } - if (xfs_is_reflink_inode(ip)) { ASSERT(ip->i_cowfp == NULL); xfs_ifork_init_cow(ip); @@ -121,18 +115,6 @@ xfs_iformat_fork( atp = (xfs_attr_shortform_t *)XFS_DFORK_APTR(dip); size = be16_to_cpu(atp->hdr.totsize); - if (unlikely(size < sizeof(struct xfs_attr_sf_hdr))) { - xfs_warn(ip->i_mount, - "corrupt inode %Lu (bad attr fork size %Ld).", - (unsigned long long) ip->i_ino, - (long long) size); - XFS_CORRUPTION_ERROR("xfs_iformat(8)", - XFS_ERRLEVEL_LOW, - ip->i_mount, dip); - error = -EFSCORRUPTED; - break; - } - error = xfs_iformat_local(ip, dip, XFS_ATTR_FORK, size); break; case XFS_DINODE_FMT_EXTENTS: @@ -740,3 +722,54 @@ xfs_ifork_init_cow( ip->i_cformat = XFS_DINODE_FMT_EXTENTS; ip->i_cnextents = 0; } + +/* 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; +} + +/* 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; +} diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h index b9f0098..dd8aba0 100644 --- a/fs/xfs/libxfs/xfs_inode_fork.h +++ b/fs/xfs/libxfs/xfs_inode_fork.h @@ -186,4 +186,18 @@ extern struct kmem_zone *xfs_ifork_zone; extern void xfs_ifork_init_cow(struct xfs_inode *ip); +typedef xfs_failaddr_t (*xfs_ifork_verifier_t)(struct xfs_inode *); + +struct xfs_ifork_ops { + xfs_ifork_verifier_t verify_symlink; + xfs_ifork_verifier_t verify_dir; + xfs_ifork_verifier_t verify_attr; +}; +extern struct xfs_ifork_ops xfs_default_ifork_ops; + +xfs_failaddr_t xfs_ifork_verify_data(struct xfs_inode *ip, + struct xfs_ifork_ops *ops); +xfs_failaddr_t xfs_ifork_verify_attr(struct xfs_inode *ip, + struct xfs_ifork_ops *ops); + #endif /* __XFS_INODE_FORK_H__ */ diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c index 43005fb..81ebb68 100644 --- a/fs/xfs/xfs_icache.c +++ b/fs/xfs/xfs_icache.c @@ -473,6 +473,11 @@ xfs_iget_cache_miss( if (error) goto out_destroy; + if (!xfs_inode_verify_forks(ip)) { + error = -EFSCORRUPTED; + goto out_destroy; + } + trace_xfs_iget_miss(ip); if ((VFS_I(ip)->i_mode == 0) && !(flags & XFS_IGET_CREATE)) { diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index 8012741..d1e2518 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -3488,6 +3488,34 @@ xfs_iflush( return error; } +/* + * If there are inline format data / attr forks attached to this inode, + * make sure they're not corrupt. + */ +bool +xfs_inode_verify_forks( + struct xfs_inode *ip) +{ + xfs_failaddr_t fa; + + fa = xfs_ifork_verify_data(ip, &xfs_default_ifork_ops); + if (fa) { + xfs_alert(ip->i_mount, + "%s: bad inode %llu inline data fork at %pF", + __func__, ip->i_ino, fa); + return false; + } + + fa = xfs_ifork_verify_attr(ip, &xfs_default_ifork_ops); + if (fa) { + xfs_alert(ip->i_mount, + "%s: bad inode %llu inline attr fork at %pF", + __func__, ip->i_ino, fa); + return false; + } + return true; +} + STATIC int xfs_iflush_int( struct xfs_inode *ip, @@ -3566,10 +3594,8 @@ xfs_iflush_int( if (ip->i_d.di_version < 3) ip->i_d.di_flushiter++; - /* Check the inline directory data. */ - if (S_ISDIR(VFS_I(ip)->i_mode) && - ip->i_d.di_format == XFS_DINODE_FMT_LOCAL && - xfs_dir2_sf_verify(ip)) + /* Check the inline fork data before we write out. */ + if (!xfs_inode_verify_forks(ip)) goto corrupt_out; /* diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h index cc13c37..e4d1708 100644 --- a/fs/xfs/xfs_inode.h +++ b/fs/xfs/xfs_inode.h @@ -490,4 +490,6 @@ extern struct kmem_zone *xfs_inode_zone; /* The default CoW extent size hint. */ #define XFS_DEFAULT_COWEXTSZ_HINT 32 +bool xfs_inode_verify_forks(struct xfs_inode *ip); + #endif /* __XFS_INODE_H__ */ diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c index 28d1abf..04f5b30 100644 --- a/fs/xfs/xfs_log_recover.c +++ b/fs/xfs/xfs_log_recover.c @@ -2957,6 +2957,10 @@ xfs_recover_inode_owner_change( if (error) goto out_free_ip; + if (!xfs_inode_verify_forks(ip)) { + error = -EFSCORRUPTED; + goto out_free_ip; + } if (in_f->ilf_fields & XFS_ILOG_DOWNER) { ASSERT(in_f->ilf_fields & XFS_ILOG_DBROOT);