[v2] xfs: move the inline directory verifiers

Commit Message

Darrick J. Wong March 27, 2017, 11:03 p.m. UTC

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 <[e-mail]>
---
v2: get the inode d_ops the proper way
---
 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(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Brian Foster March 28, 2017, 12:51 p.m. UTC | #1

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 <[e-mail]>
> ---
> 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 [e-mail]
> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Darrick J. Wong March 28, 2017, 3 p.m. UTC | #2

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 <[e-mail]>
> > ---
> > 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 [e-mail]
> > 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Brian Foster March 28, 2017, 3:11 p.m. UTC | #3

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 <[e-mail]>
> > > ---
> > > 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 [e-mail]
> > > 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 [e-mail]
> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Brian Foster March 28, 2017, 5:24 p.m. UTC | #4

On Tue, Mar 28, 2017 at 11:11:05AM -0400, Brian Foster wrote:
> 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 <[e-mail]>
> > > > ---
> > > > 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/
> 

I lost track of the fact that the first patch went into -rc and thus
confused myself over where this should apply. This applies to 4.11.0-rc4
and looks fine to me:

Reviewed-by: Brian Foster <[e-mail]>

> > --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 [e-mail]
> > > > 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 [e-mail]
> > 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 [e-mail]
> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Darrick J. Wong March 29, 2017, 6:21 p.m. UTC | #5

On Tue, Mar 28, 2017 at 01:24:44PM -0400, Brian Foster wrote:
> On Tue, Mar 28, 2017 at 11:11:05AM -0400, Brian Foster wrote:
> > 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 <[e-mail]>
> > > > > ---
> > > > > 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/
> > 
> 
> I lost track of the fact that the first patch went into -rc and thus
> confused myself over where this should apply. This applies to 4.11.0-rc4
> and looks fine to me:

Does anyone have a problem if I send this to Linus for 4.11-rc5?
I'd rather atone for my sins sooner than later. :)

> Reviewed-by: Brian Foster <[e-mail]>

Ok, thanks.

--D

> 
> > > --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 [e-mail]
> > > > > 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 [e-mail]
> > > 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 [e-mail]
> > 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 [e-mail]
> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Brian Foster March 29, 2017, 6:52 p.m. UTC | #6

On Wed, Mar 29, 2017 at 11:21:57AM -0700, Darrick J. Wong wrote:
> On Tue, Mar 28, 2017 at 01:24:44PM -0400, Brian Foster wrote:
> > On Tue, Mar 28, 2017 at 11:11:05AM -0400, Brian Foster wrote:
> > > 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 <[e-mail]>
> > > > > > ---
> > > > > > 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/
> > > 
> > 
> > I lost track of the fact that the first patch went into -rc and thus
> > confused myself over where this should apply. This applies to 4.11.0-rc4
> > and looks fine to me:
> 
> Does anyone have a problem if I send this to Linus for 4.11-rc5?
> I'd rather atone for my sins sooner than later. :)
> 

No issues from me. You might want to give Dave 24h or so to ack/nack
though..

Brian

> > Reviewed-by: Brian Foster <[e-mail]>
> 
> Ok, thanks.
> 
> --D
> 
> > 
> > > > --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 [e-mail]
> > > > > > 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 [e-mail]
> > > > 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 [e-mail]
> > > 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 [e-mail]
> > 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 [e-mail]
> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dave Chinner March 29, 2017, 10:41 p.m. UTC | #7

On Wed, Mar 29, 2017 at 11:21:57AM -0700, Darrick J. Wong wrote:
> On Tue, Mar 28, 2017 at 01:24:44PM -0400, Brian Foster wrote:
> > On Tue, Mar 28, 2017 at 11:11:05AM -0400, Brian Foster wrote:
> > > 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 <[e-mail]>
> > > > > > ---
> > > > > > 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/
> > > 
> > 
> > I lost track of the fact that the first patch went into -rc and thus
> > confused myself over where this should apply. This applies to 4.11.0-rc4
> > and looks fine to me:
> 
> Does anyone have a problem if I send this to Linus for 4.11-rc5?
> I'd rather atone for my sins sooner than later. :)

There's no urgency required here - it's just a cleanup patch. The
code in the tree works fine, so why risk adding regressions
at a late stage? Just add it to the for-next queue and let it soak
until the merge window.

Cheers,

Dave.

Christoph Hellwig March 31, 2017, 4:07 p.m. UTC | #8

On Thu, Mar 30, 2017 at 09:41:10AM +1100, Dave Chinner wrote:
> > > I lost track of the fact that the first patch went into -rc and thus
> > > confused myself over where this should apply. This applies to 4.11.0-rc4
> > > and looks fine to me:
> > 
> > Does anyone have a problem if I send this to Linus for 4.11-rc5?
> > I'd rather atone for my sins sooner than later. :)
> 
> There's no urgency required here - it's just a cleanup patch. The
> code in the tree works fine, so why risk adding regressions
> at a late stage? Just add it to the for-next queue and let it soak
> until the merge window.

Is current Linus tree ok?  I'm pretty sure a recent Linus tree fell
over when running the dir fuzzers for me.  Which commit would the
latest actual fix be?
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Darrick J. Wong March 31, 2017, 4:24 p.m. UTC | #9

On Fri, Mar 31, 2017 at 09:07:43AM -0700, Christoph Hellwig wrote:
> On Thu, Mar 30, 2017 at 09:41:10AM +1100, Dave Chinner wrote:
> > > > I lost track of the fact that the first patch went into -rc and thus
> > > > confused myself over where this should apply. This applies to 4.11.0-rc4
> > > > and looks fine to me:
> > > 
> > > Does anyone have a problem if I send this to Linus for 4.11-rc5?
> > > I'd rather atone for my sins sooner than later. :)
> > 
> > There's no urgency required here - it's just a cleanup patch. The
> > code in the tree works fine, so why risk adding regressions
> > at a late stage? Just add it to the for-next queue and let it soak
> > until the merge window.
> 
> Is current Linus tree ok?  I'm pretty sure a recent Linus tree fell
> over when running the dir fuzzers for me.  Which commit would the
> latest actual fix be?

I think xfs/348 causes -rc4 to fall over if CONFIG_XFS_DEBUG=y due to
the XFS_WANT_CORRUPTED_RETURNs that shouldn't be there.

--D

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to [e-mail]
> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Christoph Hellwig March 31, 2017, 4:28 p.m. UTC | #10

On Fri, Mar 31, 2017 at 09:24:31AM -0700, Darrick J. Wong wrote:
> I think xfs/348 causes -rc4 to fall over if CONFIG_XFS_DEBUG=y due to
> the XFS_WANT_CORRUPTED_RETURNs that shouldn't be there.

Yes, that's the one.  How are we going to fix that for -rc5?
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Darrick J. Wong March 31, 2017, 4:38 p.m. UTC | #11

On Fri, Mar 31, 2017 at 09:28:17AM -0700, Christoph Hellwig wrote:
> On Fri, Mar 31, 2017 at 09:24:31AM -0700, Darrick J. Wong wrote:
> > I think xfs/348 causes -rc4 to fall over if CONFIG_XFS_DEBUG=y due to
> > the XFS_WANT_CORRUPTED_RETURNs that shouldn't be there.
> 
> Yes, that's the one.  How are we going to fix that for -rc5?

This patch fixes all the thinkos in the original patch, so I was just
going to send it to Linus for rc5, but decided to poll the ML to see if
anyone had an objection to that.

--D

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to [e-mail]
> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Christoph Hellwig March 31, 2017, 4:40 p.m. UTC | #12

On Fri, Mar 31, 2017 at 09:38:36AM -0700, Darrick J. Wong wrote:
> On Fri, Mar 31, 2017 at 09:28:17AM -0700, Christoph Hellwig wrote:
> > On Fri, Mar 31, 2017 at 09:24:31AM -0700, Darrick J. Wong wrote:
> > > I think xfs/348 causes -rc4 to fall over if CONFIG_XFS_DEBUG=y due to
> > > the XFS_WANT_CORRUPTED_RETURNs that shouldn't be there.
> > 
> > Yes, that's the one.  How are we going to fix that for -rc5?
> 
> This patch fixes all the thinkos in the original patch, so I was just
> going to send it to Linus for rc5, but decided to poll the ML to see if
> anyone had an objection to that.

That's what I thought and was surprised by the reply from Dave..
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Christoph Hellwig March 31, 2017, 5:24 p.m. UTC | #13

Looks good,

Reviewed-by: Christoph Hellwig <[e-mail]>
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dave Chinner April 1, 2017, 11:17 p.m. UTC | #14

On Fri, Mar 31, 2017 at 09:40:01AM -0700, Christoph Hellwig wrote:
> On Fri, Mar 31, 2017 at 09:38:36AM -0700, Darrick J. Wong wrote:
> > On Fri, Mar 31, 2017 at 09:28:17AM -0700, Christoph Hellwig wrote:
> > > On Fri, Mar 31, 2017 at 09:24:31AM -0700, Darrick J. Wong wrote:
> > > > I think xfs/348 causes -rc4 to fall over if CONFIG_XFS_DEBUG=y due to
> > > > the XFS_WANT_CORRUPTED_RETURNs that shouldn't be there.
> > > 
> > > Yes, that's the one.  How are we going to fix that for -rc5?
> > 
> > This patch fixes all the thinkos in the original patch, so I was just
> > going to send it to Linus for rc5, but decided to poll the ML to see if
> > anyone had an objection to that.
> 
> That's what I thought and was surprised by the reply from Dave..

There's no mention that it fixes a bug, regression or anything else
like that in the commit message. AFAICT from the description, it's
just a cleanup to match how the rest of the checking code is
structured with better error reporting.

Perhaps the commit message needs more work, because I can't tell you
what bug this is fixing from it....

Cheers,

Dave.

Patch

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);
 
 	/*