Re: [PATCH] xfs: fix behaviour of XFS_IOC_FSSETXATTR on directories

[Dave Chinner <david@fromorbit.com>]

On Wed, Aug 27, 2014 at 09:22:53PM -0700, Iustin Pop wrote:
> Currently, the ioctl handling code for XFS_IOC_FSSETXATTR treats all
> targets as regular files: it refuses to change the extent size if
> extents are allocated. This is wrong for directories, as there the
> extent size is only used as a default for children.
> 
> The patch fixes this by only checking for allocated extents for regular
> files; it leaves undefined what it means to set an extent size on a
> non-regular, non-directory inode. Additionally, when setting a non-zero
> extent size, the appropriate flags (EXTSIZE, respectively EXTSZINHERIT)
> are enforced for regular files and directories.
> 
> Signed-off-by: Iustin Pop <iusty@k1024.org>
> ---
>  A patch against xfstests to test for the fixed behaviour will follow
>  shortly.
> 
>  fs/xfs/xfs_ioctl.c | 34 ++++++++++++++++++++++++++--------
>  1 file changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 8bc1bbc..5b9acd2 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1116,14 +1116,32 @@ xfs_ioctl_setattr(
>  	}
>  
>  	if (mask & FSX_EXTSIZE) {
> -		/*
> -		 * Can't change extent size if any extents are allocated.
> -		 */
> -		if (ip->i_d.di_nextents &&
> -		    ((ip->i_d.di_extsize << mp->m_sb.sb_blocklog) !=
> -		     fa->fsx_extsize)) {
> -			code = XFS_ERROR(EINVAL);	/* EFBIG? */
> -			goto error_return;

Doesn't apply to a current 3.17-rc1 tree. Can you update the patch?

> +		if (S_ISDIR(ip->i_d.di_mode)) {
> +			/*
> +			 * Enforce setting the EXTSZINHERIT flag when
> +			 * a non-zero extent size has been specified.
> +			 */
> +			if (fa->fsx_extsize) {
> +				fa->fsx_xflags |= XFS_XFLAG_EXTSZINHERIT;
> +			}
> +		} else if (S_ISREG(ip->i_d.di_mode)) {
> +			/*
> +			 * For a regular file, we can't change extent
> +			 * size if any extents are allocated.
> +			 */
> +			if (ip->i_d.di_nextents &&
> +			    ((ip->i_d.di_extsize << mp->m_sb.sb_blocklog) !=
> +			     fa->fsx_extsize)) {
> +				code = XFS_ERROR(EINVAL);	/* EFBIG? */
> +				goto error_return;
> +			}
> +			/*
> +			 * Enforce setting the EXTSIZE flag when
> +			 * a non-zero extent size has been specified.
> +			 */
> +			if (fa->fsx_extsize) {
> +				fa->fsx_xflags |= XFS_XFLAG_EXTSIZE;
> +			}
>  		}

Hmmmm. That's not validating/enforcing the correct use of
XFS_XFLAG_EXTSIZE or XFS_XFLAG_EXTSZINHERIT, that's setting it
implicitly based on the type of inode.

If we are going to enforce this properly, then XFS_XFLAG_EXTSIZE is
only valid for a regular file, and XFS_XFLAG_EXTSZINHERIT is only
valid on a directory, and the flags on th einode should only be set
if the hint is not zero. i.e:

	if (mask & FSX_EXTSIZE) {
		error = -EINVAL;

		/* validate the flags are set appropriately */
		if ((fa->fsx_xflags & XFS_XFLAG_EXTSIZE) &&
		    !S_ISREG(ip->i_d.di_mode))
			goto error_return;
		if ((fa->fsx_xflags & XFS_XFLAG_EXTSZINHERIT) &&
		    !S_ISDIR(ip->i_d.di_mode))
			goto error_return;

		/* Can't change extent size on regular files with allocated extents */
		if (S_ISREG(ip->i_d.di_mode) && ip->i_d.di_nextents &&
		    ((ip->i_d.di_extsize << mp->m_sb.sb_blocklog) !=
							fa->fsx_extsize))
			goto error_return;

		/* if the extent size is zero, clear the inode flags */
		if (fs->fsx_extsize == 0)
			fa->fsx_xflags &= ~(XFS_XFLAG_EXTSIZE | XFS_XFLAG_EXTSZINHERIT);
		} else {
			/* existing size check code */
			....
		}
	}

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs