xfs: rework the inline directory verifiers

Source kernel commit: 78420281a9d74014af7616958806c3aba056319e

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>
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: Eric Sandeen <sandeen@sandeen.net>

diff --git a/libxfs/util.c b/libxfs/util.c
index cd802e897a5b3cd036ecd7157fea797114068a05..dcfca3972d4f54e6e00d89942d33eacf992183c0 100644 (file)
--- a/libxfs/util.c
+++ b/libxfs/util.c
@@ -36,6 +36,9 @@
 #include "xfs_ialloc.h"
 #include "xfs_alloc.h"
 #include "xfs_bit.h"
+#include "xfs_da_format.h"
+#include "xfs_da_btree.h"
+#include "xfs_dir2_priv.h"
 
 /*
  * Calculate the worst case log unit reservation for a given superblock
@@ -433,6 +436,12 @@ libxfs_iflush_int(xfs_inode_t *ip, xfs_buf_t *bp)
        if (ip->i_d.di_version == 3)
                VFS_I(ip)->i_version++;
 
+       /* 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))
+               return -EFSCORRUPTED;
+
        /*
         * Copy the dirty parts of the inode into the on-disk
         * inode.  We always copy out the core of the inode,

diff --git a/libxfs/xfs_dir2_priv.h b/libxfs/xfs_dir2_priv.h
index eb00bc133bca673c556eb85a18385bbc3748dfcf..39f8604f764e13147fd576e95f5ad30f98dc4d1e 100644 (file)
--- a/libxfs/xfs_dir2_priv.h
+++ b/libxfs/xfs_dir2_priv.h
@@ -125,8 +125,7 @@ extern int xfs_dir2_sf_create(struct xfs_da_args *args, xfs_ino_t pino);
 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,

diff --git a/libxfs/xfs_dir2_sf.c b/libxfs/xfs_dir2_sf.c
index 712afa5ccbb6e445ad8971f241d9f877f7252928..195f816d98baa2dd118ef31f90727e17875ee0d3 100644 (file)
--- a/libxfs/xfs_dir2_sf.c
+++ b/libxfs/xfs_dir2_sf.c
@@ -630,36 +630,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 */
@@ -670,12 +683,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
@@ -683,33 +696,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;
 }

diff --git a/libxfs/xfs_inode_fork.c b/libxfs/xfs_inode_fork.c
index cc7f36bf598278c2e0e8052e87bdde6046ebcd49..814625c691399dcb3665e2f36c44fc3ff0db2915 100644 (file)
--- a/libxfs/xfs_inode_fork.c
+++ b/libxfs/xfs_inode_fork.c
@@ -209,6 +209,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);
@@ -319,8 +329,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
@@ -336,14 +344,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;
 }
@@ -864,7 +864,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,
@@ -874,7 +874,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] =
@@ -883,7 +882,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,
@@ -891,19 +890,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);
@@ -956,7 +948,6 @@ xfs_iflush_fork(
                ASSERT(0);
                break;
        }
-       return 0;
 }
 
 /*

diff --git a/libxfs/xfs_inode_fork.h b/libxfs/xfs_inode_fork.h
index 132dc59fdde6942cd22fca4ae11b8adbc193f051..7fb8365326d1a745583c4f133bc5a63668316b33 100644 (file)
--- a/libxfs/xfs_inode_fork.h
+++ b/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);