From 817b6c480e330a5325ed9acb0cef8143923a52de Mon Sep 17 00:00:00 2001 From: Dave Chinner Date: Mon, 2 Feb 2015 10:14:04 +1100 Subject: xfs: FSX_NONBLOCK is not used It is set if the filp is set ot non-blocking, but the flag is not used anywhere. Hence we can kill it. Signed-off-by: Dave Chinner Reviewed-by: Brian Foster Signed-off-by: Dave Chinner --- fs/xfs/xfs_ioctl.c | 5 ----- 1 file changed, 5 deletions(-) (limited to 'fs/xfs') diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index a1831980a68e..0c0e74f314a6 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -1016,7 +1016,6 @@ xfs_diflags_to_linux( #define FSX_PROJID 1 #define FSX_EXTSIZE 2 #define FSX_XFLAGS 4 -#define FSX_NONBLOCK 8 STATIC int xfs_ioctl_setattr( @@ -1299,8 +1298,6 @@ xfs_ioc_fssetxattr( return -EFAULT; mask = FSX_XFLAGS | FSX_EXTSIZE | FSX_PROJID; - if (filp->f_flags & (O_NDELAY|O_NONBLOCK)) - mask |= FSX_NONBLOCK; error = mnt_want_write_file(filp); if (error) @@ -1343,8 +1340,6 @@ xfs_ioc_setxflags( return -EOPNOTSUPP; mask = FSX_XFLAGS; - if (filp->f_flags & (O_NDELAY|O_NONBLOCK)) - mask |= FSX_NONBLOCK; fa.fsx_xflags = xfs_merge_ioc_xflags(flags, xfs_ip2xflags(ip)); error = mnt_want_write_file(filp); -- cgit v1.2.3 From 29a17c00d4b1b8eab61b85b71cb5a83455a7dc5e Mon Sep 17 00:00:00 2001 From: Dave Chinner Date: Mon, 2 Feb 2015 10:14:25 +1100 Subject: xfs: separate xflags from xfs_ioctl_setattr The setting of the extended flags is down through two separate interfaces, but they are munged together into xfs_ioctl_setattr and make that function far more complex than it needs to be. Separate it out into a helper function along with all the other common inode changes and transaction manipulations in xfs_ioctl_setattr(). Signed-off-by: Dave Chinner Reviewed-by: Brian Foster Signed-off-by: Dave Chinner --- fs/xfs/xfs_ioctl.c | 89 +++++++++++++++++++++++++----------------------------- 1 file changed, 41 insertions(+), 48 deletions(-) (limited to 'fs/xfs') diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index 0c0e74f314a6..b0064bdb7a6e 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -1013,6 +1013,44 @@ xfs_diflags_to_linux( inode->i_flags &= ~S_NOATIME; } +static int +xfs_ioctl_setattr_xflags( + struct xfs_trans *tp, + struct xfs_inode *ip, + struct fsxattr *fa) +{ + struct xfs_mount *mp = ip->i_mount; + + /* Can't change realtime flag if any extents are allocated. */ + if ((ip->i_d.di_nextents || ip->i_delayed_blks) && + XFS_IS_REALTIME_INODE(ip) != (fa->fsx_xflags & XFS_XFLAG_REALTIME)) + return -EINVAL; + + /* If realtime flag is set then must have realtime device */ + if (fa->fsx_xflags & XFS_XFLAG_REALTIME) { + if (mp->m_sb.sb_rblocks == 0 || mp->m_sb.sb_rextsize == 0 || + (ip->i_d.di_extsize % mp->m_sb.sb_rextsize)) + return -EINVAL; + } + + /* + * Can't modify an immutable/append-only file unless + * we have appropriate permission. + */ + if (((ip->i_d.di_flags & (XFS_DIFLAG_IMMUTABLE | XFS_DIFLAG_APPEND)) || + (fa->fsx_xflags & (XFS_XFLAG_IMMUTABLE | XFS_XFLAG_APPEND))) && + !capable(CAP_LINUX_IMMUTABLE)) + return -EPERM; + + xfs_trans_ijoin(tp, ip, 0); + xfs_set_diflags(ip, fa->fsx_xflags); + xfs_diflags_to_linux(ip); + xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG); + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); + XFS_STATS_INC(xs_ig_attrchg); + return 0; +} + #define FSX_PROJID 1 #define FSX_EXTSIZE 2 #define FSX_XFLAGS 4 @@ -1159,44 +1197,9 @@ xfs_ioctl_setattr( } - if (mask & FSX_XFLAGS) { - /* - * Can't change realtime flag if any extents are allocated. - */ - if ((ip->i_d.di_nextents || ip->i_delayed_blks) && - (XFS_IS_REALTIME_INODE(ip)) != - (fa->fsx_xflags & XFS_XFLAG_REALTIME)) { - code = -EINVAL; /* EFBIG? */ - goto error_return; - } - - /* - * If realtime flag is set then must have realtime data. - */ - if ((fa->fsx_xflags & XFS_XFLAG_REALTIME)) { - if ((mp->m_sb.sb_rblocks == 0) || - (mp->m_sb.sb_rextsize == 0) || - (ip->i_d.di_extsize % mp->m_sb.sb_rextsize)) { - code = -EINVAL; - goto error_return; - } - } - - /* - * Can't modify an immutable/append-only file unless - * we have appropriate permission. - */ - if ((ip->i_d.di_flags & - (XFS_DIFLAG_IMMUTABLE|XFS_DIFLAG_APPEND) || - (fa->fsx_xflags & - (XFS_XFLAG_IMMUTABLE | XFS_XFLAG_APPEND))) && - !capable(CAP_LINUX_IMMUTABLE)) { - code = -EPERM; - goto error_return; - } - } - - xfs_trans_ijoin(tp, ip, 0); + code = xfs_ioctl_setattr_xflags(tp, ip, fa); + if (code) + goto error_return; /* * Change file ownership. Must be the owner or privileged. @@ -1227,11 +1230,6 @@ xfs_ioctl_setattr( } - if (mask & FSX_XFLAGS) { - xfs_set_diflags(ip, fa->fsx_xflags); - xfs_diflags_to_linux(ip); - } - /* * Only set the extent size hint if we've already determined that the * extent size hint should be set on the inode. If no extent size flags @@ -1246,11 +1244,6 @@ xfs_ioctl_setattr( ip->i_d.di_extsize = extsize; } - xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG); - xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); - - XFS_STATS_INC(xs_ig_attrchg); - /* * If this is a synchronous mount, make sure that the * transaction goes to disk before returning to the user. -- cgit v1.2.3 From 8f3d17ab060ec21cead88b81c65050a6ff77e9be Mon Sep 17 00:00:00 2001 From: Dave Chinner Date: Mon, 2 Feb 2015 10:15:35 +1100 Subject: xfs: factor out xfs_ioctl_setattr transaciton preamble The setup of the transaction is done after a random smattering of checks and before another bunch of ioperations specific validity checks. Pull all the preamble out into a helper function that returns a transaction or error. Signed-off-by: Dave Chinner Reviewed-by: Brian Foster Signed-off-by: Dave Chinner --- fs/xfs/xfs_ioctl.c | 101 +++++++++++++++++++++++++++++------------------------ 1 file changed, 55 insertions(+), 46 deletions(-) (limited to 'fs/xfs') diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index b0064bdb7a6e..0f62f5b3e221 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -1042,7 +1042,6 @@ xfs_ioctl_setattr_xflags( !capable(CAP_LINUX_IMMUTABLE)) return -EPERM; - xfs_trans_ijoin(tp, ip, 0); xfs_set_diflags(ip, fa->fsx_xflags); xfs_diflags_to_linux(ip); xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG); @@ -1051,6 +1050,54 @@ xfs_ioctl_setattr_xflags( return 0; } +/* + * Set up the transaction structure for the setattr operation, checking that we + * have permission to do so. On success, return a clean transaction and the + * inode locked exclusively ready for further operation specific checks. On + * failure, return an error without modifying or locking the inode. + */ +static struct xfs_trans * +xfs_ioctl_setattr_get_trans( + struct xfs_inode *ip) +{ + struct xfs_mount *mp = ip->i_mount; + struct xfs_trans *tp; + int error; + + if (mp->m_flags & XFS_MOUNT_RDONLY) + return ERR_PTR(-EROFS); + if (XFS_FORCED_SHUTDOWN(mp)) + return ERR_PTR(-EIO); + + tp = xfs_trans_alloc(mp, XFS_TRANS_SETATTR_NOT_SIZE); + error = xfs_trans_reserve(tp, &M_RES(mp)->tr_ichange, 0, 0); + if (error) + goto out_cancel; + + xfs_ilock(ip, XFS_ILOCK_EXCL); + xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL); + + /* + * CAP_FOWNER overrides the following restrictions: + * + * The user ID of the calling process must be equal to the file owner + * ID, except in cases where the CAP_FSETID capability is applicable. + */ + if (!inode_owner_or_capable(VFS_I(ip))) { + error = -EPERM; + goto out_cancel; + } + + if (mp->m_flags & XFS_MOUNT_WSYNC) + xfs_trans_set_sync(tp); + + return tp; + +out_cancel: + xfs_trans_cancel(tp, 0); + return ERR_PTR(error); +} + #define FSX_PROJID 1 #define FSX_EXTSIZE 2 #define FSX_XFLAGS 4 @@ -1063,7 +1110,6 @@ xfs_ioctl_setattr( { struct xfs_mount *mp = ip->i_mount; struct xfs_trans *tp; - unsigned int lock_flags = 0; struct xfs_dquot *udqp = NULL; struct xfs_dquot *pdqp = NULL; struct xfs_dquot *olddquot = NULL; @@ -1071,11 +1117,6 @@ xfs_ioctl_setattr( trace_xfs_ioctl_setattr(ip); - if (mp->m_flags & XFS_MOUNT_RDONLY) - return -EROFS; - if (XFS_FORCED_SHUTDOWN(mp)) - return -EIO; - /* * Disallow 32bit project ids when projid32bit feature is not enabled. */ @@ -1099,28 +1140,10 @@ xfs_ioctl_setattr( return code; } - /* - * For the other attributes, we acquire the inode lock and - * first do an error checking pass. - */ - tp = xfs_trans_alloc(mp, XFS_TRANS_SETATTR_NOT_SIZE); - code = xfs_trans_reserve(tp, &M_RES(mp)->tr_ichange, 0, 0); - if (code) - goto error_return; - - lock_flags = XFS_ILOCK_EXCL; - xfs_ilock(ip, lock_flags); - - /* - * CAP_FOWNER overrides the following restrictions: - * - * The user ID of the calling process must be equal - * to the file owner ID, except in cases where the - * CAP_FSETID capability is applicable. - */ - if (!inode_owner_or_capable(VFS_I(ip))) { - code = -EPERM; - goto error_return; + tp = xfs_ioctl_setattr_get_trans(ip); + if (IS_ERR(tp)) { + code = PTR_ERR(tp); + goto error_free_dquots; } /* @@ -1244,20 +1267,7 @@ xfs_ioctl_setattr( ip->i_d.di_extsize = extsize; } - /* - * If this is a synchronous mount, make sure that the - * transaction goes to disk before returning to the user. - * This is slightly sub-optimal in that truncates require - * two sync transactions instead of one for wsync filesystems. - * One for the truncate and one for the timestamps since we - * don't want to change the timestamps unless we're sure the - * truncate worked. Truncates are less than 1% of the laddis - * mix so this probably isn't worth the trouble to optimize. - */ - if (mp->m_flags & XFS_MOUNT_WSYNC) - xfs_trans_set_sync(tp); code = xfs_trans_commit(tp, 0); - xfs_iunlock(ip, lock_flags); /* * Release any dquot(s) the inode had kept before chown. @@ -1268,12 +1278,11 @@ xfs_ioctl_setattr( return code; - error_return: +error_return: + xfs_trans_cancel(tp, 0); +error_free_dquots: xfs_qm_dqrele(udqp); xfs_qm_dqrele(pdqp); - xfs_trans_cancel(tp, 0); - if (lock_flags) - xfs_iunlock(ip, lock_flags); return code; } -- cgit v1.2.3 From f96291f6a39c2b60bede851efa059ba89e5f8277 Mon Sep 17 00:00:00 2001 From: Dave Chinner Date: Mon, 2 Feb 2015 10:15:56 +1100 Subject: xfs: disaggregate xfs_ioctl_setattr xfs_ioctl_setxflags doesn't need all of the functionailty in xfs_ioctl_setattr() and now we have separate helper functions that share the checks and modifications that xfs_ioctl_setxflags requires. Hence disaggregate it from xfs_ioctl_setattr() to allow further work to be done on xfs_ioctl_setattr. Signed-off-by: Dave Chinner Reviewed-by: Brian Foster Signed-off-by: Dave Chinner --- fs/xfs/xfs_ioctl.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) (limited to 'fs/xfs') diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index 0f62f5b3e221..383e61f514f7 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -1324,14 +1324,14 @@ xfs_ioc_getxflags( STATIC int xfs_ioc_setxflags( - xfs_inode_t *ip, + struct xfs_inode *ip, struct file *filp, void __user *arg) { + struct xfs_trans *tp; struct fsxattr fa; unsigned int flags; - unsigned int mask; - int error; + int error; if (copy_from_user(&flags, arg, sizeof(flags))) return -EFAULT; @@ -1341,13 +1341,26 @@ xfs_ioc_setxflags( FS_SYNC_FL)) return -EOPNOTSUPP; - mask = FSX_XFLAGS; fa.fsx_xflags = xfs_merge_ioc_xflags(flags, xfs_ip2xflags(ip)); error = mnt_want_write_file(filp); if (error) return error; - error = xfs_ioctl_setattr(ip, &fa, mask); + + tp = xfs_ioctl_setattr_get_trans(ip); + if (IS_ERR(tp)) { + error = PTR_ERR(tp); + goto out_drop_write; + } + + error = xfs_ioctl_setattr_xflags(tp, ip, &fa); + if (error) { + xfs_trans_cancel(tp, 0); + goto out_drop_write; + } + + error = xfs_trans_commit(tp, 0); +out_drop_write: mnt_drop_write_file(filp); return error; } -- cgit v1.2.3 From fd179b9c3bdab682ae5bb3e10380a31853be179a Mon Sep 17 00:00:00 2001 From: Dave Chinner Date: Mon, 2 Feb 2015 10:16:25 +1100 Subject: xfs: kill xfs_ioctl_setattr behaviour mask Now there is only one caller to xfs_ioctl_setattr that uses all the functionality of the function we can kill the behviour mask and start cleaning up the code. Signed-off-by: Dave Chinner Reviewed-by: Brian Foster Signed-off-by: Dave Chinner --- fs/xfs/xfs_ioctl.c | 168 +++++++++++++++++++++-------------------------------- 1 file changed, 65 insertions(+), 103 deletions(-) (limited to 'fs/xfs') diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index 383e61f514f7..a1f285411b74 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -1098,15 +1098,10 @@ out_cancel: return ERR_PTR(error); } -#define FSX_PROJID 1 -#define FSX_EXTSIZE 2 -#define FSX_XFLAGS 4 - STATIC int xfs_ioctl_setattr( xfs_inode_t *ip, - struct fsxattr *fa, - int mask) + struct fsxattr *fa) { struct xfs_mount *mp = ip->i_mount; struct xfs_trans *tp; @@ -1120,8 +1115,8 @@ xfs_ioctl_setattr( /* * Disallow 32bit project ids when projid32bit feature is not enabled. */ - if ((mask & FSX_PROJID) && (fa->fsx_projid > (__uint16_t)-1) && - !xfs_sb_version_hasprojid32bit(&ip->i_mount->m_sb)) + if (fa->fsx_projid > (__uint16_t)-1 && + !xfs_sb_version_hasprojid32bit(&ip->i_mount->m_sb)) return -EINVAL; /* @@ -1132,7 +1127,7 @@ xfs_ioctl_setattr( * If the IDs do change before we take the ilock, we're covered * because the i_*dquot fields will get updated anyway. */ - if (XFS_IS_QUOTA_ON(mp) && (mask & FSX_PROJID)) { + if (XFS_IS_QUOTA_ON(mp)) { code = xfs_qm_vop_dqalloc(ip, ip->i_d.di_uid, ip->i_d.di_gid, fa->fsx_projid, XFS_QMOPT_PQUOTA, &udqp, NULL, &pdqp); @@ -1151,72 +1146,53 @@ xfs_ioctl_setattr( * Only allow changing of projid from init_user_ns since it is a * non user namespace aware identifier. */ - if (mask & FSX_PROJID) { - if (current_user_ns() != &init_user_ns) { - code = -EINVAL; - goto error_return; - } - - if (XFS_IS_QUOTA_RUNNING(mp) && - XFS_IS_PQUOTA_ON(mp) && - xfs_get_projid(ip) != fa->fsx_projid) { - ASSERT(tp); - code = xfs_qm_vop_chown_reserve(tp, ip, udqp, NULL, - pdqp, capable(CAP_FOWNER) ? - XFS_QMOPT_FORCE_RES : 0); - if (code) /* out of quota */ - goto error_return; - } + if (current_user_ns() != &init_user_ns) { + code = -EINVAL; + goto error_return; } - 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 = -EINVAL; /* EFBIG? */ + if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_PQUOTA_ON(mp) && + xfs_get_projid(ip) != fa->fsx_projid) { + code = xfs_qm_vop_chown_reserve(tp, ip, udqp, NULL, pdqp, + capable(CAP_FOWNER) ? XFS_QMOPT_FORCE_RES : 0); + if (code) /* out of quota */ goto error_return; - } + } - /* - * Extent size must be a multiple of the appropriate block - * size, if set at all. It must also be smaller than the - * maximum extent size supported by the filesystem. - * - * Also, for non-realtime files, limit the extent size hint to - * half the size of the AGs in the filesystem so alignment - * doesn't result in extents larger than an AG. - */ - if (fa->fsx_extsize != 0) { - xfs_extlen_t size; - xfs_fsblock_t extsize_fsb; + /* Can't change extent size if any extents are allocated. */ + code = -EINVAL; + if (ip->i_d.di_nextents && + ((ip->i_d.di_extsize << mp->m_sb.sb_blocklog) != fa->fsx_extsize)) + goto error_return; - extsize_fsb = XFS_B_TO_FSB(mp, fa->fsx_extsize); - if (extsize_fsb > MAXEXTLEN) { - code = -EINVAL; - goto error_return; - } - - if (XFS_IS_REALTIME_INODE(ip) || - ((mask & FSX_XFLAGS) && - (fa->fsx_xflags & XFS_XFLAG_REALTIME))) { - size = mp->m_sb.sb_rextsize << - mp->m_sb.sb_blocklog; - } else { - size = mp->m_sb.sb_blocksize; - if (extsize_fsb > mp->m_sb.sb_agblocks / 2) { - code = -EINVAL; - goto error_return; - } - } - - if (fa->fsx_extsize % size) { - code = -EINVAL; + /* + * Extent size must be a multiple of the appropriate block size, if set + * at all. It must also be smaller than the maximum extent size + * supported by the filesystem. + * + * Also, for non-realtime files, limit the extent size hint to half the + * size of the AGs in the filesystem so alignment doesn't result in + * extents larger than an AG. + */ + if (fa->fsx_extsize != 0) { + xfs_extlen_t size; + xfs_fsblock_t extsize_fsb; + + extsize_fsb = XFS_B_TO_FSB(mp, fa->fsx_extsize); + if (extsize_fsb > MAXEXTLEN) + goto error_return; + + if (XFS_IS_REALTIME_INODE(ip) || + (fa->fsx_xflags & XFS_XFLAG_REALTIME)) { + size = mp->m_sb.sb_rextsize << mp->m_sb.sb_blocklog; + } else { + size = mp->m_sb.sb_blocksize; + if (extsize_fsb > mp->m_sb.sb_agblocks / 2) goto error_return; - } } + + if (fa->fsx_extsize % size) + goto error_return; } @@ -1225,32 +1201,25 @@ xfs_ioctl_setattr( goto error_return; /* - * Change file ownership. Must be the owner or privileged. + * Change file ownership. Must be the owner or privileged. CAP_FSETID + * overrides the following restrictions: + * + * The set-user-ID and set-group-ID bits of a file will be cleared upon + * successful return from chown() */ - if (mask & FSX_PROJID) { - /* - * CAP_FSETID overrides the following restrictions: - * - * The set-user-ID and set-group-ID bits of a file will be - * cleared upon successful return from chown() - */ - if ((ip->i_d.di_mode & (S_ISUID|S_ISGID)) && - !capable_wrt_inode_uidgid(VFS_I(ip), CAP_FSETID)) - ip->i_d.di_mode &= ~(S_ISUID|S_ISGID); - /* - * Change the ownerships and register quota modifications - * in the transaction. - */ - if (xfs_get_projid(ip) != fa->fsx_projid) { - if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_PQUOTA_ON(mp)) { - olddquot = xfs_qm_vop_chown(tp, ip, - &ip->i_pdquot, pdqp); - } - ASSERT(ip->i_d.di_version > 1); - xfs_set_projid(ip, fa->fsx_projid); - } + if ((ip->i_d.di_mode & (S_ISUID|S_ISGID)) && + !capable_wrt_inode_uidgid(VFS_I(ip), CAP_FSETID)) + ip->i_d.di_mode &= ~(S_ISUID|S_ISGID); + /* Change the ownerships and register project quota modifications */ + if (xfs_get_projid(ip) != fa->fsx_projid) { + if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_PQUOTA_ON(mp)) { + olddquot = xfs_qm_vop_chown(tp, ip, + &ip->i_pdquot, pdqp); + } + ASSERT(ip->i_d.di_version > 1); + xfs_set_projid(ip, fa->fsx_projid); } /* @@ -1258,14 +1227,10 @@ xfs_ioctl_setattr( * extent size hint should be set on the inode. If no extent size flags * are set on the inode then unconditionally clear the extent size hint. */ - if (mask & FSX_EXTSIZE) { - int extsize = 0; - - if (ip->i_d.di_flags & - (XFS_DIFLAG_EXTSIZE | XFS_DIFLAG_EXTSZINHERIT)) - extsize = fa->fsx_extsize >> mp->m_sb.sb_blocklog; - ip->i_d.di_extsize = extsize; - } + if (ip->i_d.di_flags & (XFS_DIFLAG_EXTSIZE | XFS_DIFLAG_EXTSZINHERIT)) + ip->i_d.di_extsize = fa->fsx_extsize >> mp->m_sb.sb_blocklog; + else + ip->i_d.di_extsize = 0; code = xfs_trans_commit(tp, 0); @@ -1293,18 +1258,15 @@ xfs_ioc_fssetxattr( void __user *arg) { struct fsxattr fa; - unsigned int mask; int error; if (copy_from_user(&fa, arg, sizeof(fa))) return -EFAULT; - mask = FSX_XFLAGS | FSX_EXTSIZE | FSX_PROJID; - error = mnt_want_write_file(filp); if (error) return error; - error = xfs_ioctl_setattr(ip, &fa, mask); + error = xfs_ioctl_setattr(ip, &fa); mnt_drop_write_file(filp); return error; } -- cgit v1.2.3 From 41c145271d79eae508321340b727d3e3c9a66664 Mon Sep 17 00:00:00 2001 From: Dave Chinner Date: Mon, 2 Feb 2015 10:17:51 +1100 Subject: xfs: XFS_IOCTL_SETXATTR can run in user namespaces Currently XFS_IOCTL_SETXATTR will fail if run in a user namespace as it it not allowed to change project IDs. The current code, however, also prevents any other change being made as well, so things like extent size hints cannot be set in user namespaces. This is wrong, so only disallow access to project IDs and related flags from inside the init namespace. Signed-off-by: Dave Chinner Reviewed-by: Brian Foster Signed-off-by: Dave Chinner --- fs/xfs/xfs_ioctl.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) (limited to 'fs/xfs') diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index a1f285411b74..b65817cbc318 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -1119,6 +1119,19 @@ xfs_ioctl_setattr( !xfs_sb_version_hasprojid32bit(&ip->i_mount->m_sb)) return -EINVAL; + /* + * Project Quota ID state is only allowed to change from within the init + * namespace. Enforce that restriction only if we are trying to change + * the quota ID state. Everything else is allowed in user namespaces. + */ + if (current_user_ns() != &init_user_ns) { + if (xfs_get_projid(ip) != fa->fsx_projid) + return -EINVAL; + if ((fa->fsx_xflags & XFS_XFLAG_PROJINHERIT) != + (ip->i_d.di_flags & XFS_DIFLAG_PROJINHERIT)) + return -EINVAL; + } + /* * If disk quotas is on, we make sure that the dquots do exist on disk, * before we start any other transactions. Trying to do this later @@ -1141,15 +1154,6 @@ xfs_ioctl_setattr( goto error_free_dquots; } - /* - * Do a quota reservation only if projid is actually going to change. - * Only allow changing of projid from init_user_ns since it is a - * non user namespace aware identifier. - */ - if (current_user_ns() != &init_user_ns) { - code = -EINVAL; - goto error_return; - } if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_PQUOTA_ON(mp) && xfs_get_projid(ip) != fa->fsx_projid) { -- cgit v1.2.3 From d4388d3c0988ec00787ad1f8e63b5e2a6abef1dc Mon Sep 17 00:00:00 2001 From: Dave Chinner Date: Mon, 2 Feb 2015 10:22:20 +1100 Subject: xfs: factor extsize hint checking out of xfs_ioctl_setattr The extent size hint change checking is fairly complex, so isolate that into it's own function. This simplifies the logic flow of the setattr code, making it easier to read. Signed-off-by: Dave Chinner Reviewed-by: Brian Foster Signed-off-by: Dave Chinner --- fs/xfs/xfs_ioctl.c | 90 +++++++++++++++++++++++++++++++----------------------- 1 file changed, 51 insertions(+), 39 deletions(-) (limited to 'fs/xfs') diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index b65817cbc318..9f808539fc61 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -1098,6 +1098,51 @@ out_cancel: return ERR_PTR(error); } +int +xfs_ioctl_setattr_check_extsize( + struct xfs_inode *ip, + struct fsxattr *fa) +{ + struct xfs_mount *mp = ip->i_mount; + + /* 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)) + return -EINVAL; + + /* + * Extent size must be a multiple of the appropriate block size, if set + * at all. It must also be smaller than the maximum extent size + * supported by the filesystem. + * + * Also, for non-realtime files, limit the extent size hint to half the + * size of the AGs in the filesystem so alignment doesn't result in + * extents larger than an AG. + */ + if (fa->fsx_extsize != 0) { + xfs_extlen_t size; + xfs_fsblock_t extsize_fsb; + + extsize_fsb = XFS_B_TO_FSB(mp, fa->fsx_extsize); + if (extsize_fsb > MAXEXTLEN) + return -EINVAL; + + if (XFS_IS_REALTIME_INODE(ip) || + (fa->fsx_xflags & XFS_XFLAG_REALTIME)) { + size = mp->m_sb.sb_rextsize << mp->m_sb.sb_blocklog; + } else { + size = mp->m_sb.sb_blocksize; + if (extsize_fsb > mp->m_sb.sb_agblocks / 2) + return -EINVAL; + } + + if (fa->fsx_extsize % size) + return -EINVAL; + } + return 0; +} + + STATIC int xfs_ioctl_setattr( xfs_inode_t *ip, @@ -1160,49 +1205,16 @@ xfs_ioctl_setattr( code = xfs_qm_vop_chown_reserve(tp, ip, udqp, NULL, pdqp, capable(CAP_FOWNER) ? XFS_QMOPT_FORCE_RES : 0); if (code) /* out of quota */ - goto error_return; - } - - /* Can't change extent size if any extents are allocated. */ - code = -EINVAL; - if (ip->i_d.di_nextents && - ((ip->i_d.di_extsize << mp->m_sb.sb_blocklog) != fa->fsx_extsize)) - goto error_return; - - /* - * Extent size must be a multiple of the appropriate block size, if set - * at all. It must also be smaller than the maximum extent size - * supported by the filesystem. - * - * Also, for non-realtime files, limit the extent size hint to half the - * size of the AGs in the filesystem so alignment doesn't result in - * extents larger than an AG. - */ - if (fa->fsx_extsize != 0) { - xfs_extlen_t size; - xfs_fsblock_t extsize_fsb; - - extsize_fsb = XFS_B_TO_FSB(mp, fa->fsx_extsize); - if (extsize_fsb > MAXEXTLEN) - goto error_return; - - if (XFS_IS_REALTIME_INODE(ip) || - (fa->fsx_xflags & XFS_XFLAG_REALTIME)) { - size = mp->m_sb.sb_rextsize << mp->m_sb.sb_blocklog; - } else { - size = mp->m_sb.sb_blocksize; - if (extsize_fsb > mp->m_sb.sb_agblocks / 2) - goto error_return; - } - - if (fa->fsx_extsize % size) - goto error_return; + goto error_trans_cancel; } + code = xfs_ioctl_setattr_check_extsize(ip, fa); + if (code) + goto error_trans_cancel; code = xfs_ioctl_setattr_xflags(tp, ip, fa); if (code) - goto error_return; + goto error_trans_cancel; /* * Change file ownership. Must be the owner or privileged. CAP_FSETID @@ -1247,7 +1259,7 @@ xfs_ioctl_setattr( return code; -error_return: +error_trans_cancel: xfs_trans_cancel(tp, 0); error_free_dquots: xfs_qm_dqrele(udqp); -- cgit v1.2.3 From 23bd0735cfdf5322170a9ef48c7d47c2e6567ba8 Mon Sep 17 00:00:00 2001 From: Dave Chinner Date: Mon, 2 Feb 2015 10:22:53 +1100 Subject: xfs: factor projid hint checking out of xfs_ioctl_setattr The project ID change checking is one of the few remaining open coded checks in xfs_ioctl_setattr(). Factor it into a helper function so that the setattr code mostly becomes a flow of check and action helpers, making it easier to read and follow. Signed-off-by: Dave Chinner Reviewed-by: Brian Foster Signed-off-by: Dave Chinner --- fs/xfs/xfs_ioctl.c | 50 +++++++++++++++++++++++++++++++------------------- 1 file changed, 31 insertions(+), 19 deletions(-) (limited to 'fs/xfs') diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index 9f808539fc61..1f186d2eec06 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -1142,6 +1142,34 @@ xfs_ioctl_setattr_check_extsize( return 0; } +int +xfs_ioctl_setattr_check_projid( + struct xfs_inode *ip, + struct fsxattr *fa) +{ + /* Disallow 32bit project ids if projid32bit feature is not enabled. */ + if (fa->fsx_projid > (__uint16_t)-1 && + !xfs_sb_version_hasprojid32bit(&ip->i_mount->m_sb)) + return -EINVAL; + + /* + * Project Quota ID state is only allowed to change from within the init + * namespace. Enforce that restriction only if we are trying to change + * the quota ID state. Everything else is allowed in user namespaces. + */ + if (current_user_ns() == &init_user_ns) + return 0; + + if (xfs_get_projid(ip) != fa->fsx_projid) + return -EINVAL; + if ((fa->fsx_xflags & XFS_XFLAG_PROJINHERIT) != + (ip->i_d.di_flags & XFS_DIFLAG_PROJINHERIT)) + return -EINVAL; + + return 0; +} + + STATIC int xfs_ioctl_setattr( @@ -1157,25 +1185,9 @@ xfs_ioctl_setattr( trace_xfs_ioctl_setattr(ip); - /* - * Disallow 32bit project ids when projid32bit feature is not enabled. - */ - if (fa->fsx_projid > (__uint16_t)-1 && - !xfs_sb_version_hasprojid32bit(&ip->i_mount->m_sb)) - return -EINVAL; - - /* - * Project Quota ID state is only allowed to change from within the init - * namespace. Enforce that restriction only if we are trying to change - * the quota ID state. Everything else is allowed in user namespaces. - */ - if (current_user_ns() != &init_user_ns) { - if (xfs_get_projid(ip) != fa->fsx_projid) - return -EINVAL; - if ((fa->fsx_xflags & XFS_XFLAG_PROJINHERIT) != - (ip->i_d.di_flags & XFS_DIFLAG_PROJINHERIT)) - return -EINVAL; - } + code = xfs_ioctl_setattr_check_projid(ip, fa); + if (code) + return code; /* * If disk quotas is on, we make sure that the dquots do exist on disk, -- cgit v1.2.3 From 9b94fcc39822b450af823b3d8cbef6b53ce87ed9 Mon Sep 17 00:00:00 2001 From: Iustin Pop Date: Mon, 2 Feb 2015 10:26:26 +1100 Subject: xfs: fix behaviour of XFS_IOC_FSSETXATTR on directories 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 issue and improves validation of flag combinations: - only disallow extent size changes after extents have been allocated for regular files - only allow XFS_XFLAG_EXTSIZE for regular files - only allow XFS_XFLAG_EXTSZINHERIT for directories - automatically clear the flags if the extent size is zero Thanks to Dave Chinner for guidance on the proper fix for this issue. [dchinner: ported changes onto cleanup series. Makes changes clear and obvious.] [dchinner: added comments documenting validity checking rules.] Signed-off-by: Iustin Pop Signed-off-by: Dave Chinner Reviewed-by: Brian Foster Signed-off-by: Dave Chinner --- fs/xfs/xfs_ioctl.c | 39 +++++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 14 deletions(-) (limited to 'fs/xfs') diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index 1f186d2eec06..0f6b6abb7c7a 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -1098,6 +1098,20 @@ out_cancel: return ERR_PTR(error); } +/* + * extent size hint validation is somewhat cumbersome. Rules are: + * + * 1. extent size hint is only valid for directories and regular files + * 2. XFS_XFLAG_EXTSIZE is only valid for regular files + * 3. XFS_XFLAG_EXTSZINHERIT is only valid for directories. + * 4. can only be changed on regular files if no extents are allocated + * 5. can be changed on directories at any time + * 6. extsize hint of 0 turns off hints, clears inode flags. + * 7. Extent size must be a multiple of the appropriate block size. + * 8. for non-realtime files, the extent size hint must be limited + * to half the AG size to avoid alignment extending the extent beyond the + * limits of the AG. + */ int xfs_ioctl_setattr_check_extsize( struct xfs_inode *ip, @@ -1105,20 +1119,17 @@ xfs_ioctl_setattr_check_extsize( { struct xfs_mount *mp = ip->i_mount; - /* Can't change extent size if any extents are allocated. */ - if (ip->i_d.di_nextents && + if ((fa->fsx_xflags & XFS_XFLAG_EXTSIZE) && !S_ISREG(ip->i_d.di_mode)) + return -EINVAL; + + if ((fa->fsx_xflags & XFS_XFLAG_EXTSZINHERIT) && + !S_ISDIR(ip->i_d.di_mode)) + return -EINVAL; + + 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)) return -EINVAL; - /* - * Extent size must be a multiple of the appropriate block size, if set - * at all. It must also be smaller than the maximum extent size - * supported by the filesystem. - * - * Also, for non-realtime files, limit the extent size hint to half the - * size of the AGs in the filesystem so alignment doesn't result in - * extents larger than an AG. - */ if (fa->fsx_extsize != 0) { xfs_extlen_t size; xfs_fsblock_t extsize_fsb; @@ -1138,7 +1149,9 @@ xfs_ioctl_setattr_check_extsize( if (fa->fsx_extsize % size) return -EINVAL; - } + } else + fa->fsx_xflags &= ~(XFS_XFLAG_EXTSIZE | XFS_XFLAG_EXTSZINHERIT); + return 0; } @@ -1169,8 +1182,6 @@ xfs_ioctl_setattr_check_projid( return 0; } - - STATIC int xfs_ioctl_setattr( xfs_inode_t *ip, -- cgit v1.2.3