| Message ID | 20190409031929.GE5147@magnolia (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Tue, Apr 9, 2019 at 6:19 AM Darrick J. Wong <darrick.wong@oracle.com> wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > The chattr manpage has this to say about immutable files: > > "A file with the 'i' attribute cannot be modified: it cannot be deleted > or renamed, no link can be created to this file, most of the file's > metadata can not be modified, and the file can not be opened in write > mode." > > However, we don't actually check the immutable flag in the setattr code, > which means that we can update project ids and extent size hints on > supposedly immutable files. Therefore, reject a setattr call on an > immutable file except for the case where we're trying to unset > IMMUTABLE. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/xfs/xfs_ioctl.c | 46 ++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 44 insertions(+), 2 deletions(-) > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > index 5a1b96dad901..67d12027f563 100644 > --- a/fs/xfs/xfs_ioctl.c > +++ b/fs/xfs/xfs_ioctl.c > @@ -1023,6 +1023,40 @@ xfs_ioctl_setattr_flush( > return filemap_write_and_wait(inode->i_mapping); > } > > +/* > + * If immutable is set and we are not clearing it, we're not allowed to change > + * anything else in the inode. This looks correct, but FYI, neither xfs_io nor chattr clears 'immutable' and sets projid/*extsize in one ioctl/xfsctl, so there is no justification to making an extra effort to support that use case. You could do with checking 'immutable' inside xfs_ioctl_setattr_check_projid/*extsize() and leave only the di_flags check here. Some would say that will be cleaner code. Its a matter of taste and its your subsystem, so feel free to dismiss this comments. Thanks, Amir. > Don't error out if we're only trying to set > + * immutable on an immutable file. > + */ > +static int > +xfs_ioctl_setattr_immutable( > + struct xfs_inode *ip, > + struct fsxattr *fa, > + uint16_t di_flags, > + uint64_t di_flags2) > +{ > + struct xfs_mount *mp = ip->i_mount; > + > + if (!(ip->i_d.di_flags & XFS_DIFLAG_IMMUTABLE) || > + !(fa->fsx_xflags & FS_XFLAG_IMMUTABLE)) > + return 0; > + > + if ((ip->i_d.di_flags & ~XFS_DIFLAG_IMMUTABLE) != > + (di_flags & ~XFS_DIFLAG_IMMUTABLE)) > + return -EPERM; > + if (ip->i_d.di_version >= 3 && ip->i_d.di_flags2 != di_flags2) > + return -EPERM; > + if (xfs_get_projid(ip) != fa->fsx_projid) > + return -EPERM; > + if (ip->i_d.di_extsize != fa->fsx_extsize >> mp->m_sb.sb_blocklog) > + return -EPERM; > + if (ip->i_d.di_version >= 3 && (di_flags2 & XFS_DIFLAG2_COWEXTSIZE) && > + ip->i_d.di_cowextsize != fa->fsx_cowextsize >> mp->m_sb.sb_blocklog) > + return -EPERM; > + > + return 0; > +} > + > static int > xfs_ioctl_setattr_xflags( > struct xfs_trans *tp, > @@ -1030,7 +1064,9 @@ xfs_ioctl_setattr_xflags( > struct fsxattr *fa) > { > struct xfs_mount *mp = ip->i_mount; > + uint16_t di_flags; > uint64_t di_flags2; > + int error; > > /* Can't change realtime flag if any extents are allocated. */ > if ((ip->i_d.di_nextents || ip->i_delayed_blks) && > @@ -1061,12 +1097,18 @@ xfs_ioctl_setattr_xflags( > !capable(CAP_LINUX_IMMUTABLE)) > return -EPERM; > > - /* diflags2 only valid for v3 inodes. */ > + /* Don't allow changes to an immutable inode. */ > + di_flags = xfs_flags2diflags(ip, fa->fsx_xflags); > di_flags2 = xfs_flags2diflags2(ip, fa->fsx_xflags); > + error = xfs_ioctl_setattr_immutable(ip, fa, di_flags, di_flags2); > + if (error) > + return error; > + > + /* diflags2 only valid for v3 inodes. */ > if (di_flags2 && ip->i_d.di_version < 3) > return -EINVAL; > > - ip->i_d.di_flags = xfs_flags2diflags(ip, fa->fsx_xflags); > + ip->i_d.di_flags = di_flags; > ip->i_d.di_flags2 = di_flags2; > > xfs_diflags_to_linux(ip);
On Tue, Apr 09, 2019 at 11:24:10AM +0300, Amir Goldstein wrote: > On Tue, Apr 9, 2019 at 6:19 AM Darrick J. Wong <darrick.wong@oracle.com> wrote: > > > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > The chattr manpage has this to say about immutable files: > > > > "A file with the 'i' attribute cannot be modified: it cannot be deleted > > or renamed, no link can be created to this file, most of the file's > > metadata can not be modified, and the file can not be opened in write > > mode." > > > > However, we don't actually check the immutable flag in the setattr code, > > which means that we can update project ids and extent size hints on > > supposedly immutable files. Therefore, reject a setattr call on an > > immutable file except for the case where we're trying to unset > > IMMUTABLE. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > fs/xfs/xfs_ioctl.c | 46 ++++++++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 44 insertions(+), 2 deletions(-) > > > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > > index 5a1b96dad901..67d12027f563 100644 > > --- a/fs/xfs/xfs_ioctl.c > > +++ b/fs/xfs/xfs_ioctl.c > > @@ -1023,6 +1023,40 @@ xfs_ioctl_setattr_flush( > > return filemap_write_and_wait(inode->i_mapping); > > } > > > > +/* > > + * If immutable is set and we are not clearing it, we're not allowed to change > > + * anything else in the inode. > > This looks correct, but FYI, neither xfs_io nor chattr clears 'immutable' > and sets projid/*extsize in one ioctl/xfsctl, so there is no justification to > making an extra effort to support that use case. You could do with > checking 'immutable' inside xfs_ioctl_setattr_check_projid/*extsize() > and leave only the di_flags check here. However, the API does allow callers to clear immutable and set other fields in one go, so just because xfs_io won't do it doesn't mean we can ignore it. Then again I guess the manpage doesn't explicitly say what the behavior is supposed to be, so I guess we'll just ... argh, fine I'll go fix the manpage to document the behavior. --D > Some would say that will be cleaner code. > Its a matter of taste and its your subsystem, so feel free to dismiss > this comments. > > Thanks, > Amir. > > > Don't error out if we're only trying to set > > + * immutable on an immutable file. > > + */ > > +static int > > +xfs_ioctl_setattr_immutable( > > + struct xfs_inode *ip, > > + struct fsxattr *fa, > > + uint16_t di_flags, > > + uint64_t di_flags2) > > +{ > > + struct xfs_mount *mp = ip->i_mount; > > + > > + if (!(ip->i_d.di_flags & XFS_DIFLAG_IMMUTABLE) || > > + !(fa->fsx_xflags & FS_XFLAG_IMMUTABLE)) > > + return 0; > > + > > + if ((ip->i_d.di_flags & ~XFS_DIFLAG_IMMUTABLE) != > > + (di_flags & ~XFS_DIFLAG_IMMUTABLE)) > > + return -EPERM; > > + if (ip->i_d.di_version >= 3 && ip->i_d.di_flags2 != di_flags2) > > + return -EPERM; > > + if (xfs_get_projid(ip) != fa->fsx_projid) > > + return -EPERM; > > + if (ip->i_d.di_extsize != fa->fsx_extsize >> mp->m_sb.sb_blocklog) > > + return -EPERM; > > + if (ip->i_d.di_version >= 3 && (di_flags2 & XFS_DIFLAG2_COWEXTSIZE) && > > + ip->i_d.di_cowextsize != fa->fsx_cowextsize >> mp->m_sb.sb_blocklog) > > + return -EPERM; > > + > > + return 0; > > +} > > + > > static int > > xfs_ioctl_setattr_xflags( > > struct xfs_trans *tp, > > @@ -1030,7 +1064,9 @@ xfs_ioctl_setattr_xflags( > > struct fsxattr *fa) > > { > > struct xfs_mount *mp = ip->i_mount; > > + uint16_t di_flags; > > uint64_t di_flags2; > > + int error; > > > > /* Can't change realtime flag if any extents are allocated. */ > > if ((ip->i_d.di_nextents || ip->i_delayed_blks) && > > @@ -1061,12 +1097,18 @@ xfs_ioctl_setattr_xflags( > > !capable(CAP_LINUX_IMMUTABLE)) > > return -EPERM; > > > > - /* diflags2 only valid for v3 inodes. */ > > + /* Don't allow changes to an immutable inode. */ > > + di_flags = xfs_flags2diflags(ip, fa->fsx_xflags); > > di_flags2 = xfs_flags2diflags2(ip, fa->fsx_xflags); > > + error = xfs_ioctl_setattr_immutable(ip, fa, di_flags, di_flags2); > > + if (error) > > + return error; > > + > > + /* diflags2 only valid for v3 inodes. */ > > if (di_flags2 && ip->i_d.di_version < 3) > > return -EINVAL; > > > > - ip->i_d.di_flags = xfs_flags2diflags(ip, fa->fsx_xflags); > > + ip->i_d.di_flags = di_flags; > > ip->i_d.di_flags2 = di_flags2; > > > > xfs_diflags_to_linux(ip);
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index 5a1b96dad901..67d12027f563 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -1023,6 +1023,40 @@ xfs_ioctl_setattr_flush( return filemap_write_and_wait(inode->i_mapping); } +/* + * If immutable is set and we are not clearing it, we're not allowed to change + * anything else in the inode. Don't error out if we're only trying to set + * immutable on an immutable file. + */ +static int +xfs_ioctl_setattr_immutable( + struct xfs_inode *ip, + struct fsxattr *fa, + uint16_t di_flags, + uint64_t di_flags2) +{ + struct xfs_mount *mp = ip->i_mount; + + if (!(ip->i_d.di_flags & XFS_DIFLAG_IMMUTABLE) || + !(fa->fsx_xflags & FS_XFLAG_IMMUTABLE)) + return 0; + + if ((ip->i_d.di_flags & ~XFS_DIFLAG_IMMUTABLE) != + (di_flags & ~XFS_DIFLAG_IMMUTABLE)) + return -EPERM; + if (ip->i_d.di_version >= 3 && ip->i_d.di_flags2 != di_flags2) + return -EPERM; + if (xfs_get_projid(ip) != fa->fsx_projid) + return -EPERM; + if (ip->i_d.di_extsize != fa->fsx_extsize >> mp->m_sb.sb_blocklog) + return -EPERM; + if (ip->i_d.di_version >= 3 && (di_flags2 & XFS_DIFLAG2_COWEXTSIZE) && + ip->i_d.di_cowextsize != fa->fsx_cowextsize >> mp->m_sb.sb_blocklog) + return -EPERM; + + return 0; +} + static int xfs_ioctl_setattr_xflags( struct xfs_trans *tp, @@ -1030,7 +1064,9 @@ xfs_ioctl_setattr_xflags( struct fsxattr *fa) { struct xfs_mount *mp = ip->i_mount; + uint16_t di_flags; uint64_t di_flags2; + int error; /* Can't change realtime flag if any extents are allocated. */ if ((ip->i_d.di_nextents || ip->i_delayed_blks) && @@ -1061,12 +1097,18 @@ xfs_ioctl_setattr_xflags( !capable(CAP_LINUX_IMMUTABLE)) return -EPERM; - /* diflags2 only valid for v3 inodes. */ + /* Don't allow changes to an immutable inode. */ + di_flags = xfs_flags2diflags(ip, fa->fsx_xflags); di_flags2 = xfs_flags2diflags2(ip, fa->fsx_xflags); + error = xfs_ioctl_setattr_immutable(ip, fa, di_flags, di_flags2); + if (error) + return error; + + /* diflags2 only valid for v3 inodes. */ if (di_flags2 && ip->i_d.di_version < 3) return -EINVAL; - ip->i_d.di_flags = xfs_flags2diflags(ip, fa->fsx_xflags); + ip->i_d.di_flags = di_flags; ip->i_d.di_flags2 = di_flags2; xfs_diflags_to_linux(ip);