xfs
[Top] [All Lists]

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

To: Iustin Pop <iustin@xxxxxxxxxx>
Subject: Re: [PATCH] xfs: fix behaviour of XFS_IOC_FSSETXATTR on directories
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 29 Aug 2014 10:46:07 +1000
Cc: Iustin Pop <iusty@xxxxxxxxx>, xfs@xxxxxxxxxxx, hch@xxxxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20140828223414.GC29940@xxxxxxxxxx>
References: <20140718191314.GB27801@xxxxxxxxxxxxxxxxx> <1409199773-16802-1-git-send-email-iusty@xxxxxxxxx> <20140828093158.GR20518@dastard> <20140828223414.GC29940@xxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Thu, Aug 28, 2014 at 03:34:14PM -0700, Iustin Pop wrote:
> On Don, Aug 28, 2014 at 07:31:58 +1000, Dave Chinner wrote:
> > 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?
> 
> Will do, thanks. Is it correct to use git://oss.sgi.com/xfs/xfs, master
> branch as what I should rebase it on top of?

I'm moving off oss.sgi.com. I'll keep the trees up to date for a
while, but the master kernel tree I'm using now is:

git://git.kernel.org/pub/scm/linux/kernel/git/dgc/linux-xfs.git master

> > > +         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;
> 
> This only prevents invalid flags, but doesn't prevent setting a non-zero
> extent_size without the appropriate flag. We already are having this
> discussion in the other patch, so I'll defer this to that.

So take the previous code I cleared the flags on extsize == 0, and
do this:

-       if (mask & FSX_EXTSIZE)
+       if (fa->fsx_flags & (XFS_XFLAG_EXTSIZE | XFS_XFLAG_EXTSZINHERIT)) {
                ip->i_d.di_extsize = fa->fsx_extsize >> mp->m_sb.sb_blocklog;
+               if (ip->i_d.di_extsize == 0)
+                       fa->fsx_flags &= ~(XFS_XFLAG_EXTSIZE | 
XFS_XFLAG_EXTSZINHERIT);
+       }

The way that mask gets used is just nasty, IMO. We could clean that
code up a lot with a bit of factoring.

> But aside from that, the current code that actually applies the flag
> changes (xfs_set_diflags) implicitly drops invalid flags - that's why I
> didn't bother to validate the flags here.

Which points out that the flag validation code is broken, yes?

> If we want to do validation,
> maybe we should validate (with error, not silently) in that function,
> instead of here?

No. We need to have a clear separation of user input validation from
the modification of the inode state, because if we abort
modification during a transaction due to a validation failure after
modifying the inode, then we have to abort the dirty transaction and
that will cause a filesystem shutdown. Which would then be called a
"user level DOS" and hence we must maintain the separation of
validation from modification that we already have...

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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