From: Brian Foster <bfoster@redhat.com>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: Dave Chinner <david@fromorbit.com>, xfs <linux-xfs@vger.kernel.org>
Subject: Re: [PATCH v2] xfs: move the inline directory verifiers
Date: Tue, 28 Mar 2017 11:11:05 -0400 [thread overview]
Message-ID: <20170328151105.GC4100@bfoster.bfoster> (raw)
In-Reply-To: <20170328150047.GB4874@birch.djwong.org>
On Tue, Mar 28, 2017 at 08:00:47AM -0700, Darrick J. Wong wrote:
> 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@oracle.com>
> > > ---
> > > 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.
>
Hm, doesn't apply against [1] for me. Care to just repost these as a
series if the dependent code hasn't been merged yet?
Brian
[1] https://patchwork.kernel.org/patch/9626859/
> --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@vger.kernel.org
> > > 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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2017-03-28 15:11 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-27 23:03 [PATCH v2] xfs: move the inline directory verifiers Darrick J. Wong
2017-03-28 12:51 ` Brian Foster
2017-03-28 15:00 ` Darrick J. Wong
2017-03-28 15:11 ` Brian Foster [this message]
2017-03-28 17:24 ` Brian Foster
2017-03-29 18:21 ` Darrick J. Wong
2017-03-29 18:52 ` Brian Foster
2017-03-29 22:41 ` Dave Chinner
2017-03-31 16:07 ` Christoph Hellwig
2017-03-31 16:24 ` Darrick J. Wong
2017-03-31 16:28 ` Christoph Hellwig
2017-03-31 16:38 ` Darrick J. Wong
2017-03-31 16:40 ` Christoph Hellwig
2017-04-01 23:17 ` Dave Chinner
2017-03-31 17:24 ` Christoph Hellwig
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170328151105.GC4100@bfoster.bfoster \
--to=bfoster@redhat.com \
--cc=darrick.wong@oracle.com \
--cc=david@fromorbit.com \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.