On Tue, Mar 28, 2017 at 08:51:05AM -0400, Brian Foster wrote:
> 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?
It ought to apply against the previous inline dir verifier patch.
--D
>
> 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
