Commit 005c5db8 authored by Darrick J. Wong's avatar Darrick J. Wong

xfs: rework the inline directory verifiers

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: default avatarBrian Foster <bfoster@redhat.com>
Reviewed-by: default avatarChristoph Hellwig <hch@lst.de>
Signed-off-by: default avatarDarrick J. Wong <darrick.wong@oracle.com>
---
v2: get the inode d_ops the proper way
v3: describe the bug that this patch fixes; no code changes
parent c02ed2e7
...@@ -125,8 +125,7 @@ extern int xfs_dir2_sf_create(struct xfs_da_args *args, xfs_ino_t pino); ...@@ -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_lookup(struct xfs_da_args *args);
extern int xfs_dir2_sf_removename(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_replace(struct xfs_da_args *args);
extern int xfs_dir2_sf_verify(struct xfs_mount *mp, struct xfs_dir2_sf_hdr *sfp, extern int xfs_dir2_sf_verify(struct xfs_inode *ip);
int size);
/* xfs_dir2_readdir.c */ /* xfs_dir2_readdir.c */
extern int xfs_readdir(struct xfs_inode *dp, struct dir_context *ctx, extern int xfs_readdir(struct xfs_inode *dp, struct dir_context *ctx,
......
...@@ -632,36 +632,49 @@ xfs_dir2_sf_check( ...@@ -632,36 +632,49 @@ xfs_dir2_sf_check(
/* Verify the consistency of an inline directory. */ /* Verify the consistency of an inline directory. */
int int
xfs_dir2_sf_verify( xfs_dir2_sf_verify(
struct xfs_mount *mp, struct xfs_inode *ip)
struct xfs_dir2_sf_hdr *sfp,
int size)
{ {
struct xfs_mount *mp = ip->i_mount;
struct xfs_dir2_sf_hdr *sfp;
struct xfs_dir2_sf_entry *sfep; struct xfs_dir2_sf_entry *sfep;
struct xfs_dir2_sf_entry *next_sfep; struct xfs_dir2_sf_entry *next_sfep;
char *endp; char *endp;
const struct xfs_dir_ops *dops; const struct xfs_dir_ops *dops;
struct xfs_ifork *ifp;
xfs_ino_t ino; xfs_ino_t ino;
int i; int i;
int i8count; int i8count;
int offset; int offset;
int size;
int error;
__uint8_t filetype; __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); 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. * Give up if the directory is way too short.
*/ */
XFS_WANT_CORRUPTED_RETURN(mp, size > if (size <= offsetof(struct xfs_dir2_sf_hdr, parent) ||
offsetof(struct xfs_dir2_sf_hdr, parent)); size < xfs_dir2_sf_hdr_size(sfp->i8count))
XFS_WANT_CORRUPTED_RETURN(mp, size >= return -EFSCORRUPTED;
xfs_dir2_sf_hdr_size(sfp->i8count));
endp = (char *)sfp + size; endp = (char *)sfp + size;
/* Check .. entry */ /* Check .. entry */
ino = dops->sf_get_parent_ino(sfp); ino = dops->sf_get_parent_ino(sfp);
i8count = ino > XFS_DIR2_MAX_SHORT_INUM; 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; offset = dops->data_first_offset;
/* Check all reported entries */ /* Check all reported entries */
...@@ -672,12 +685,12 @@ xfs_dir2_sf_verify( ...@@ -672,12 +685,12 @@ xfs_dir2_sf_verify(
* Check the fixed-offset parts of the structure are * Check the fixed-offset parts of the structure are
* within the data buffer. * within the data buffer.
*/ */
XFS_WANT_CORRUPTED_RETURN(mp, if (((char *)sfep + sizeof(*sfep)) >= endp)
((char *)sfep + sizeof(*sfep)) < endp); return -EFSCORRUPTED;
/* Don't allow names with known bad length. */ /* Don't allow names with known bad length. */
XFS_WANT_CORRUPTED_RETURN(mp, sfep->namelen > 0); if (sfep->namelen == 0)
XFS_WANT_CORRUPTED_RETURN(mp, sfep->namelen < MAXNAMELEN); return -EFSCORRUPTED;
/* /*
* Check that the variable-length part of the structure is * Check that the variable-length part of the structure is
...@@ -685,33 +698,39 @@ xfs_dir2_sf_verify( ...@@ -685,33 +698,39 @@ xfs_dir2_sf_verify(
* name component, so nextentry is an acceptable test. * name component, so nextentry is an acceptable test.
*/ */
next_sfep = dops->sf_nextentry(sfp, sfep); 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. */ /* Check that the offsets always increase. */
XFS_WANT_CORRUPTED_RETURN(mp, if (xfs_dir2_sf_get_offset(sfep) < offset)
xfs_dir2_sf_get_offset(sfep) >= offset); return -EFSCORRUPTED;
/* Check the inode number. */ /* Check the inode number. */
ino = dops->sf_get_ino(sfp, sfep); ino = dops->sf_get_ino(sfp, sfep);
i8count += ino > XFS_DIR2_MAX_SHORT_INUM; 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. */ /* Check the file type. */
filetype = dops->sf_get_ftype(sfep); 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) + offset = xfs_dir2_sf_get_offset(sfep) +
dops->data_entsize(sfep->namelen); dops->data_entsize(sfep->namelen);
sfep = next_sfep; sfep = next_sfep;
} }
XFS_WANT_CORRUPTED_RETURN(mp, i8count == sfp->i8count); if (i8count != sfp->i8count)
XFS_WANT_CORRUPTED_RETURN(mp, (void *)sfep == (void *)endp); return -EFSCORRUPTED;
if ((void *)sfep != (void *)endp)
return -EFSCORRUPTED;
/* Make sure this whole thing ought to be in local format. */ /* Make sure this whole thing ought to be in local format. */
XFS_WANT_CORRUPTED_RETURN(mp, offset + if (offset + (sfp->count + 2) * (uint)sizeof(xfs_dir2_leaf_entry_t) +
(sfp->count + 2) * (uint)sizeof(xfs_dir2_leaf_entry_t) + (uint)sizeof(xfs_dir2_block_tail_t) > mp->m_dir_geo->blksize)
(uint)sizeof(xfs_dir2_block_tail_t) <= mp->m_dir_geo->blksize); return -EFSCORRUPTED;
return 0; return 0;
} }
......
...@@ -212,6 +212,16 @@ xfs_iformat_fork( ...@@ -212,6 +212,16 @@ xfs_iformat_fork(
if (error) if (error)
return 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)) { if (xfs_is_reflink_inode(ip)) {
ASSERT(ip->i_cowfp == NULL); ASSERT(ip->i_cowfp == NULL);
xfs_ifork_init_cow(ip); xfs_ifork_init_cow(ip);
...@@ -322,8 +332,6 @@ xfs_iformat_local( ...@@ -322,8 +332,6 @@ xfs_iformat_local(
int whichfork, int whichfork,
int size) int size)
{ {
int error;
/* /*
* If the size is unreasonable, then something * If the size is unreasonable, then something
* is wrong and we just bail out rather than crash in * is wrong and we just bail out rather than crash in
...@@ -339,14 +347,6 @@ xfs_iformat_local( ...@@ -339,14 +347,6 @@ xfs_iformat_local(
return -EFSCORRUPTED; 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); xfs_init_local_fork(ip, whichfork, XFS_DFORK_PTR(dip, whichfork), size);
return 0; return 0;
} }
...@@ -867,7 +867,7 @@ xfs_iextents_copy( ...@@ -867,7 +867,7 @@ xfs_iextents_copy(
* In these cases, the format always takes precedence, because the * In these cases, the format always takes precedence, because the
* format indicates the current state of the fork. * format indicates the current state of the fork.
*/ */
int void
xfs_iflush_fork( xfs_iflush_fork(
xfs_inode_t *ip, xfs_inode_t *ip,
xfs_dinode_t *dip, xfs_dinode_t *dip,
...@@ -877,7 +877,6 @@ xfs_iflush_fork( ...@@ -877,7 +877,6 @@ xfs_iflush_fork(
char *cp; char *cp;
xfs_ifork_t *ifp; xfs_ifork_t *ifp;
xfs_mount_t *mp; xfs_mount_t *mp;
int error;
static const short brootflag[2] = static const short brootflag[2] =
{ XFS_ILOG_DBROOT, XFS_ILOG_ABROOT }; { XFS_ILOG_DBROOT, XFS_ILOG_ABROOT };
static const short dataflag[2] = static const short dataflag[2] =
...@@ -886,7 +885,7 @@ xfs_iflush_fork( ...@@ -886,7 +885,7 @@ xfs_iflush_fork(
{ XFS_ILOG_DEXT, XFS_ILOG_AEXT }; { XFS_ILOG_DEXT, XFS_ILOG_AEXT };
if (!iip) if (!iip)
return 0; return;
ifp = XFS_IFORK_PTR(ip, whichfork); ifp = XFS_IFORK_PTR(ip, whichfork);
/* /*
* This can happen if we gave up in iformat in an error path, * This can happen if we gave up in iformat in an error path,
...@@ -894,19 +893,12 @@ xfs_iflush_fork( ...@@ -894,19 +893,12 @@ xfs_iflush_fork(
*/ */
if (!ifp) { if (!ifp) {
ASSERT(whichfork == XFS_ATTR_FORK); ASSERT(whichfork == XFS_ATTR_FORK);
return 0; return;
} }
cp = XFS_DFORK_PTR(dip, whichfork); cp = XFS_DFORK_PTR(dip, whichfork);
mp = ip->i_mount; mp = ip->i_mount;
switch (XFS_IFORK_FORMAT(ip, whichfork)) { switch (XFS_IFORK_FORMAT(ip, whichfork)) {
case XFS_DINODE_FMT_LOCAL: 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]) && if ((iip->ili_fields & dataflag[whichfork]) &&
(ifp->if_bytes > 0)) { (ifp->if_bytes > 0)) {
ASSERT(ifp->if_u1.if_data != NULL); ASSERT(ifp->if_u1.if_data != NULL);
...@@ -959,7 +951,6 @@ xfs_iflush_fork( ...@@ -959,7 +951,6 @@ xfs_iflush_fork(
ASSERT(0); ASSERT(0);
break; break;
} }
return 0;
} }
/* /*
......
...@@ -140,7 +140,7 @@ typedef struct xfs_ifork { ...@@ -140,7 +140,7 @@ typedef struct xfs_ifork {
struct xfs_ifork *xfs_iext_state_to_fork(struct xfs_inode *ip, int state); 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_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); struct xfs_inode_log_item *, int);
void xfs_idestroy_fork(struct xfs_inode *, int); void xfs_idestroy_fork(struct xfs_inode *, int);
void xfs_idata_realloc(struct xfs_inode *, int, int); void xfs_idata_realloc(struct xfs_inode *, int, int);
......
...@@ -50,6 +50,7 @@ ...@@ -50,6 +50,7 @@
#include "xfs_log.h" #include "xfs_log.h"
#include "xfs_bmap_btree.h" #include "xfs_bmap_btree.h"
#include "xfs_reflink.h" #include "xfs_reflink.h"
#include "xfs_dir2_priv.h"
kmem_zone_t *xfs_inode_zone; kmem_zone_t *xfs_inode_zone;
...@@ -3475,7 +3476,6 @@ xfs_iflush_int( ...@@ -3475,7 +3476,6 @@ xfs_iflush_int(
struct xfs_inode_log_item *iip = ip->i_itemp; struct xfs_inode_log_item *iip = ip->i_itemp;
struct xfs_dinode *dip; struct xfs_dinode *dip;
struct xfs_mount *mp = ip->i_mount; struct xfs_mount *mp = ip->i_mount;
int error;
ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)); ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
ASSERT(xfs_isiflocked(ip)); ASSERT(xfs_isiflocked(ip));
...@@ -3547,6 +3547,12 @@ xfs_iflush_int( ...@@ -3547,6 +3547,12 @@ xfs_iflush_int(
if (ip->i_d.di_version < 3) if (ip->i_d.di_version < 3)
ip->i_d.di_flushiter++; 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 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 * copy out the core of the inode, because if the inode is dirty at all
...@@ -3558,14 +3564,9 @@ xfs_iflush_int( ...@@ -3558,14 +3564,9 @@ xfs_iflush_int(
if (ip->i_d.di_flushiter == DI_MAX_FLUSH) if (ip->i_d.di_flushiter == DI_MAX_FLUSH)
ip->i_d.di_flushiter = 0; ip->i_d.di_flushiter = 0;
error = xfs_iflush_fork(ip, dip, iip, XFS_DATA_FORK); xfs_iflush_fork(ip, dip, iip, XFS_DATA_FORK);
if (error) if (XFS_IFORK_Q(ip))
return error; xfs_iflush_fork(ip, dip, iip, XFS_ATTR_FORK);
if (XFS_IFORK_Q(ip)) {
error = xfs_iflush_fork(ip, dip, iip, XFS_ATTR_FORK);
if (error)
return error;
}
xfs_inobp_check(mp, bp); xfs_inobp_check(mp, bp);
/* /*
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment