On Mon, Mar 27, 2017 at 04:03:15PM -0700, Darrick J. Wong wrote:
> 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.
>
> Also revise the verifier function to return -EFSCORRUPTED so that we
> don't flood the logs with corruption messages and assert notices.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ---
> v2: get the inode d_ops the proper way
> ---
What does this apply against?
Brian
> 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(-)
>
> diff --git a/fs/xfs/libxfs/xfs_dir2_priv.h b/fs/xfs/libxfs/xfs_dir2_priv.h
> index eb00bc1..39f8604 100644
> --- a/fs/xfs/libxfs/xfs_dir2_priv.h
> +++ b/fs/xfs/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/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> index 96b45cd..e84af09 100644
> --- 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;
> }
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 9653e96..8a37efe 100644
> --- 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;
> }
>
> /*
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index 132dc59..7fb8365 100644
> --- 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);
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index c7fe2c2..7605d83 100644
> --- 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;
>
> @@ -3475,7 +3476,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));
> @@ -3547,6 +3547,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
> @@ -3558,14 +3564,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);
>
> /*
> --
> 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
