| From 78420281a9d74014af7616958806c3aba056319e Mon Sep 17 00:00:00 2001 |
| From: "Darrick J. Wong" <darrick.wong@oracle.com> |
| Date: Mon, 3 Apr 2017 12:22:20 -0700 |
| Subject: xfs: rework the inline directory verifiers |
| |
| From: Darrick J. Wong <darrick.wong@oracle.com> |
| |
| commit 78420281a9d74014af7616958806c3aba056319e upstream. |
| |
| The inline directory verifiers should be called on the inode fork data, |
| which means after iformat_local on the read side, and prior to |
| ifork_flush on the write side. This makes the fork verifier more |
| consistent with the way buffer verifiers work -- i.e. they will operate |
| on the memory buffer that the code will be reading and writing directly. |
| |
| Furthermore, revise the verifier function to return -EFSCORRUPTED so |
| that we don't flood the logs with corruption messages and assert |
| notices. This has been a particular problem with xfs/348, which |
| triggers the XFS_WANT_CORRUPTED_RETURN assertions, which halts the |
| kernel when CONFIG_XFS_DEBUG=y. Disk corruption isn't supposed to do |
| that, at least not in a verifier. |
| |
| Reviewed-by: Brian Foster <bfoster@redhat.com> |
| Reviewed-by: Christoph Hellwig <hch@lst.de> |
| Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| |
| --- |
| fs/xfs/libxfs/xfs_dir2_priv.h | 3 - |
| fs/xfs/libxfs/xfs_dir2_sf.c | 63 ++++++++++++++++++++++++++--------------- |
| fs/xfs/libxfs/xfs_inode_fork.c | 35 ++++++++-------------- |
| fs/xfs/libxfs/xfs_inode_fork.h | 2 - |
| fs/xfs/xfs_inode.c | 19 ++++++------ |
| 5 files changed, 66 insertions(+), 56 deletions(-) |
| |
| --- a/fs/xfs/libxfs/xfs_dir2_priv.h |
| +++ b/fs/xfs/libxfs/xfs_dir2_priv.h |
| @@ -126,8 +126,7 @@ extern int xfs_dir2_sf_create(struct xfs |
| extern int xfs_dir2_sf_lookup(struct xfs_da_args *args); |
| extern int xfs_dir2_sf_removename(struct xfs_da_args *args); |
| extern int xfs_dir2_sf_replace(struct xfs_da_args *args); |
| -extern int xfs_dir2_sf_verify(struct xfs_mount *mp, struct xfs_dir2_sf_hdr *sfp, |
| - int size); |
| +extern int xfs_dir2_sf_verify(struct xfs_inode *ip); |
| |
| /* xfs_dir2_readdir.c */ |
| extern int xfs_readdir(struct xfs_inode *dp, struct dir_context *ctx, |
| --- a/fs/xfs/libxfs/xfs_dir2_sf.c |
| +++ b/fs/xfs/libxfs/xfs_dir2_sf.c |
| @@ -632,36 +632,49 @@ xfs_dir2_sf_check( |
| /* Verify the consistency of an inline directory. */ |
| int |
| xfs_dir2_sf_verify( |
| - struct xfs_mount *mp, |
| - struct xfs_dir2_sf_hdr *sfp, |
| - int size) |
| + struct xfs_inode *ip) |
| { |
| + struct xfs_mount *mp = ip->i_mount; |
| + struct xfs_dir2_sf_hdr *sfp; |
| struct xfs_dir2_sf_entry *sfep; |
| struct xfs_dir2_sf_entry *next_sfep; |
| char *endp; |
| const struct xfs_dir_ops *dops; |
| + struct xfs_ifork *ifp; |
| xfs_ino_t ino; |
| int i; |
| int i8count; |
| int offset; |
| + int size; |
| + int error; |
| __uint8_t filetype; |
| |
| + ASSERT(ip->i_d.di_format == XFS_DINODE_FMT_LOCAL); |
| + /* |
| + * xfs_iread calls us before xfs_setup_inode sets up ip->d_ops, |
| + * so we can only trust the mountpoint to have the right pointer. |
| + */ |
| dops = xfs_dir_get_ops(mp, NULL); |
| |
| + ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK); |
| + sfp = (struct xfs_dir2_sf_hdr *)ifp->if_u1.if_data; |
| + size = ifp->if_bytes; |
| + |
| /* |
| * Give up if the directory is way too short. |
| */ |
| - XFS_WANT_CORRUPTED_RETURN(mp, size > |
| - offsetof(struct xfs_dir2_sf_hdr, parent)); |
| - XFS_WANT_CORRUPTED_RETURN(mp, size >= |
| - xfs_dir2_sf_hdr_size(sfp->i8count)); |
| + if (size <= offsetof(struct xfs_dir2_sf_hdr, parent) || |
| + size < xfs_dir2_sf_hdr_size(sfp->i8count)) |
| + return -EFSCORRUPTED; |
| |
| endp = (char *)sfp + size; |
| |
| /* Check .. entry */ |
| ino = dops->sf_get_parent_ino(sfp); |
| i8count = ino > XFS_DIR2_MAX_SHORT_INUM; |
| - XFS_WANT_CORRUPTED_RETURN(mp, !xfs_dir_ino_validate(mp, ino)); |
| + error = xfs_dir_ino_validate(mp, ino); |
| + if (error) |
| + return error; |
| offset = dops->data_first_offset; |
| |
| /* Check all reported entries */ |
| @@ -672,12 +685,12 @@ xfs_dir2_sf_verify( |
| * Check the fixed-offset parts of the structure are |
| * within the data buffer. |
| */ |
| - XFS_WANT_CORRUPTED_RETURN(mp, |
| - ((char *)sfep + sizeof(*sfep)) < endp); |
| + if (((char *)sfep + sizeof(*sfep)) >= endp) |
| + return -EFSCORRUPTED; |
| |
| /* Don't allow names with known bad length. */ |
| - XFS_WANT_CORRUPTED_RETURN(mp, sfep->namelen > 0); |
| - XFS_WANT_CORRUPTED_RETURN(mp, sfep->namelen < MAXNAMELEN); |
| + if (sfep->namelen == 0) |
| + return -EFSCORRUPTED; |
| |
| /* |
| * Check that the variable-length part of the structure is |
| @@ -685,33 +698,39 @@ xfs_dir2_sf_verify( |
| * name component, so nextentry is an acceptable test. |
| */ |
| next_sfep = dops->sf_nextentry(sfp, sfep); |
| - XFS_WANT_CORRUPTED_RETURN(mp, endp >= (char *)next_sfep); |
| + if (endp < (char *)next_sfep) |
| + return -EFSCORRUPTED; |
| |
| /* Check that the offsets always increase. */ |
| - XFS_WANT_CORRUPTED_RETURN(mp, |
| - xfs_dir2_sf_get_offset(sfep) >= offset); |
| + if (xfs_dir2_sf_get_offset(sfep) < offset) |
| + return -EFSCORRUPTED; |
| |
| /* Check the inode number. */ |
| ino = dops->sf_get_ino(sfp, sfep); |
| i8count += ino > XFS_DIR2_MAX_SHORT_INUM; |
| - XFS_WANT_CORRUPTED_RETURN(mp, !xfs_dir_ino_validate(mp, ino)); |
| + error = xfs_dir_ino_validate(mp, ino); |
| + if (error) |
| + return error; |
| |
| /* Check the file type. */ |
| filetype = dops->sf_get_ftype(sfep); |
| - XFS_WANT_CORRUPTED_RETURN(mp, filetype < XFS_DIR3_FT_MAX); |
| + if (filetype >= XFS_DIR3_FT_MAX) |
| + return -EFSCORRUPTED; |
| |
| offset = xfs_dir2_sf_get_offset(sfep) + |
| dops->data_entsize(sfep->namelen); |
| |
| sfep = next_sfep; |
| } |
| - XFS_WANT_CORRUPTED_RETURN(mp, i8count == sfp->i8count); |
| - XFS_WANT_CORRUPTED_RETURN(mp, (void *)sfep == (void *)endp); |
| + if (i8count != sfp->i8count) |
| + return -EFSCORRUPTED; |
| + if ((void *)sfep != (void *)endp) |
| + return -EFSCORRUPTED; |
| |
| /* Make sure this whole thing ought to be in local format. */ |
| - XFS_WANT_CORRUPTED_RETURN(mp, offset + |
| - (sfp->count + 2) * (uint)sizeof(xfs_dir2_leaf_entry_t) + |
| - (uint)sizeof(xfs_dir2_block_tail_t) <= mp->m_dir_geo->blksize); |
| + if (offset + (sfp->count + 2) * (uint)sizeof(xfs_dir2_leaf_entry_t) + |
| + (uint)sizeof(xfs_dir2_block_tail_t) > mp->m_dir_geo->blksize) |
| + return -EFSCORRUPTED; |
| |
| return 0; |
| } |
| --- a/fs/xfs/libxfs/xfs_inode_fork.c |
| +++ b/fs/xfs/libxfs/xfs_inode_fork.c |
| @@ -212,6 +212,16 @@ xfs_iformat_fork( |
| if (error) |
| return error; |
| |
| + /* Check inline dir contents. */ |
| + if (S_ISDIR(VFS_I(ip)->i_mode) && |
| + dip->di_format == XFS_DINODE_FMT_LOCAL) { |
| + error = xfs_dir2_sf_verify(ip); |
| + if (error) { |
| + xfs_idestroy_fork(ip, XFS_DATA_FORK); |
| + return error; |
| + } |
| + } |
| + |
| if (xfs_is_reflink_inode(ip)) { |
| ASSERT(ip->i_cowfp == NULL); |
| xfs_ifork_init_cow(ip); |
| @@ -322,8 +332,6 @@ xfs_iformat_local( |
| int whichfork, |
| int size) |
| { |
| - int error; |
| - |
| /* |
| * If the size is unreasonable, then something |
| * is wrong and we just bail out rather than crash in |
| @@ -339,14 +347,6 @@ xfs_iformat_local( |
| return -EFSCORRUPTED; |
| } |
| |
| - if (S_ISDIR(VFS_I(ip)->i_mode) && whichfork == XFS_DATA_FORK) { |
| - error = xfs_dir2_sf_verify(ip->i_mount, |
| - (struct xfs_dir2_sf_hdr *)XFS_DFORK_DPTR(dip), |
| - size); |
| - if (error) |
| - return error; |
| - } |
| - |
| xfs_init_local_fork(ip, whichfork, XFS_DFORK_PTR(dip, whichfork), size); |
| return 0; |
| } |
| @@ -867,7 +867,7 @@ xfs_iextents_copy( |
| * In these cases, the format always takes precedence, because the |
| * format indicates the current state of the fork. |
| */ |
| -int |
| +void |
| xfs_iflush_fork( |
| xfs_inode_t *ip, |
| xfs_dinode_t *dip, |
| @@ -877,7 +877,6 @@ xfs_iflush_fork( |
| char *cp; |
| xfs_ifork_t *ifp; |
| xfs_mount_t *mp; |
| - int error; |
| static const short brootflag[2] = |
| { XFS_ILOG_DBROOT, XFS_ILOG_ABROOT }; |
| static const short dataflag[2] = |
| @@ -886,7 +885,7 @@ xfs_iflush_fork( |
| { XFS_ILOG_DEXT, XFS_ILOG_AEXT }; |
| |
| if (!iip) |
| - return 0; |
| + return; |
| ifp = XFS_IFORK_PTR(ip, whichfork); |
| /* |
| * This can happen if we gave up in iformat in an error path, |
| @@ -894,19 +893,12 @@ xfs_iflush_fork( |
| */ |
| if (!ifp) { |
| ASSERT(whichfork == XFS_ATTR_FORK); |
| - return 0; |
| + return; |
| } |
| cp = XFS_DFORK_PTR(dip, whichfork); |
| mp = ip->i_mount; |
| switch (XFS_IFORK_FORMAT(ip, whichfork)) { |
| case XFS_DINODE_FMT_LOCAL: |
| - if (S_ISDIR(VFS_I(ip)->i_mode) && whichfork == XFS_DATA_FORK) { |
| - error = xfs_dir2_sf_verify(mp, |
| - (struct xfs_dir2_sf_hdr *)ifp->if_u1.if_data, |
| - ifp->if_bytes); |
| - if (error) |
| - return error; |
| - } |
| if ((iip->ili_fields & dataflag[whichfork]) && |
| (ifp->if_bytes > 0)) { |
| ASSERT(ifp->if_u1.if_data != NULL); |
| @@ -959,7 +951,6 @@ xfs_iflush_fork( |
| ASSERT(0); |
| break; |
| } |
| - return 0; |
| } |
| |
| /* |
| --- a/fs/xfs/libxfs/xfs_inode_fork.h |
| +++ b/fs/xfs/libxfs/xfs_inode_fork.h |
| @@ -140,7 +140,7 @@ typedef struct xfs_ifork { |
| struct xfs_ifork *xfs_iext_state_to_fork(struct xfs_inode *ip, int state); |
| |
| int xfs_iformat_fork(struct xfs_inode *, struct xfs_dinode *); |
| -int xfs_iflush_fork(struct xfs_inode *, struct xfs_dinode *, |
| +void xfs_iflush_fork(struct xfs_inode *, struct xfs_dinode *, |
| struct xfs_inode_log_item *, int); |
| void xfs_idestroy_fork(struct xfs_inode *, int); |
| void xfs_idata_realloc(struct xfs_inode *, int, int); |
| --- a/fs/xfs/xfs_inode.c |
| +++ b/fs/xfs/xfs_inode.c |
| @@ -50,6 +50,7 @@ |
| #include "xfs_log.h" |
| #include "xfs_bmap_btree.h" |
| #include "xfs_reflink.h" |
| +#include "xfs_dir2_priv.h" |
| |
| kmem_zone_t *xfs_inode_zone; |
| |
| @@ -3491,7 +3492,6 @@ xfs_iflush_int( |
| struct xfs_inode_log_item *iip = ip->i_itemp; |
| struct xfs_dinode *dip; |
| struct xfs_mount *mp = ip->i_mount; |
| - int error; |
| |
| ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)); |
| ASSERT(xfs_isiflocked(ip)); |
| @@ -3563,6 +3563,12 @@ 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)) |
| + goto corrupt_out; |
| + |
| /* |
| * Copy the dirty parts of the inode into the on-disk inode. We always |
| * copy out the core of the inode, because if the inode is dirty at all |
| @@ -3574,14 +3580,9 @@ xfs_iflush_int( |
| if (ip->i_d.di_flushiter == DI_MAX_FLUSH) |
| ip->i_d.di_flushiter = 0; |
| |
| - error = xfs_iflush_fork(ip, dip, iip, XFS_DATA_FORK); |
| - if (error) |
| - return error; |
| - if (XFS_IFORK_Q(ip)) { |
| - error = xfs_iflush_fork(ip, dip, iip, XFS_ATTR_FORK); |
| - if (error) |
| - return error; |
| - } |
| + xfs_iflush_fork(ip, dip, iip, XFS_DATA_FORK); |
| + if (XFS_IFORK_Q(ip)) |
| + xfs_iflush_fork(ip, dip, iip, XFS_ATTR_FORK); |
| xfs_inobp_check(mp, bp); |
| |
| /* |