xfs
[Top] [All Lists]

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

To: Iustin Pop <iusty@xxxxxxxxx>
Subject: Re: [PATCH] xfs: fix behaviour of XFS_IOC_FSSETXATTR on directories
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 28 Aug 2014 19:31:58 +1000
Cc: xfs@xxxxxxxxxxx, hch@xxxxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1409199773-16802-1-git-send-email-iusty@xxxxxxxxx>
References: <20140718191314.GB27801@xxxxxxxxxxxxxxxxx> <1409199773-16802-1-git-send-email-iusty@xxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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@xxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>