* [PATCH 0/9] xfs: xfs_ioctl_setxattr rework
@ 2015-01-27 3:14 Dave Chinner
2015-01-27 3:14 ` [PATCH 1/9] xfs: FSX_NONBLOCK is not used Dave Chinner
` (9 more replies)
0 siblings, 10 replies; 23+ messages in thread
From: Dave Chinner @ 2015-01-27 3:14 UTC (permalink / raw)
To: xfs; +Cc: iustin
Hi folks,
This is a series I started a few months ago when we first started
talking about the issues with extent size hints on directories and
the project ID inherit flags being set on regular files. The code
is particularly nice and has no clear definition of what sort
of changes we allow in the XFS_IOC_FSSETXATTR ioctl.
The first thing the series does is kill the FSX_* flags and separate
out the two different use cases for the xfs_ioctl_setattr()
function. The first is just changing a constrained set of flags via
the xfs_ioc_setxflags(), and the second is supporting
xfs_ioc_fssetxattr(). Factoring out the part of the code that sets
just the inode flags appropriately allows us to kill the FSX_* flags
completely.
The next patch then relaxes the overly defensive approach to
restricting XFS_IOC_FSSETXATTR to only the init namespace. We really
only need to restrict project ID changes - allowing changes to other
parts of the inode are managed by user/group permissions which are
already user namespace aware.
The next part of the patch set factors out the validity checking
of extent size changes and project ID changes from the setattr
functions, making it much clearer the separation between checks and
actions performed by xfs_ioctl_setattr() function.
Finally, with all these changes, Iustin Pop's extent size change
validity checking patch is ported on top. That now becomes a simple,
obvious set of changes to an isolated function, and i've added
comments to explain the rules allowing extent size hints to be
changed.
Comments, thoughts, flames, etc all welcome.
- Dave.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 1/9] xfs: FSX_NONBLOCK is not used
2015-01-27 3:14 [PATCH 0/9] xfs: xfs_ioctl_setxattr rework Dave Chinner
@ 2015-01-27 3:14 ` Dave Chinner
2015-01-29 15:33 ` Brian Foster
2015-01-27 3:14 ` [PATCH 2/9] xfs: separate xflags from xfs_ioctl_setattr Dave Chinner
` (8 subsequent siblings)
9 siblings, 1 reply; 23+ messages in thread
From: Dave Chinner @ 2015-01-27 3:14 UTC (permalink / raw)
To: xfs; +Cc: iustin
From: Dave Chinner <dchinner@redhat.com>
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 <dchinner@redhat.com>
---
fs/xfs/xfs_ioctl.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 8810959..09325ff 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);
--
2.0.0
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 2/9] xfs: separate xflags from xfs_ioctl_setattr
2015-01-27 3:14 [PATCH 0/9] xfs: xfs_ioctl_setxattr rework Dave Chinner
2015-01-27 3:14 ` [PATCH 1/9] xfs: FSX_NONBLOCK is not used Dave Chinner
@ 2015-01-27 3:14 ` Dave Chinner
2015-01-29 15:34 ` Brian Foster
2015-01-27 3:14 ` [PATCH 3/9] xfs: factor out xfs_ioctl_setattr transaciton preamble Dave Chinner
` (7 subsequent siblings)
9 siblings, 1 reply; 23+ messages in thread
From: Dave Chinner @ 2015-01-27 3:14 UTC (permalink / raw)
To: xfs; +Cc: iustin
From: Dave Chinner <dchinner@redhat.com>
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 <dchinner@redhat.com>
---
fs/xfs/xfs_ioctl.c | 89 +++++++++++++++++++++++++-----------------------------
1 file changed, 41 insertions(+), 48 deletions(-)
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 09325ff..ba98412 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.
--
2.0.0
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 3/9] xfs: factor out xfs_ioctl_setattr transaciton preamble
2015-01-27 3:14 [PATCH 0/9] xfs: xfs_ioctl_setxattr rework Dave Chinner
2015-01-27 3:14 ` [PATCH 1/9] xfs: FSX_NONBLOCK is not used Dave Chinner
2015-01-27 3:14 ` [PATCH 2/9] xfs: separate xflags from xfs_ioctl_setattr Dave Chinner
@ 2015-01-27 3:14 ` Dave Chinner
2015-01-29 15:34 ` Brian Foster
2015-01-27 3:14 ` [PATCH 4/9] xfs: disaggregate xfs_ioctl_setattr Dave Chinner
` (6 subsequent siblings)
9 siblings, 1 reply; 23+ messages in thread
From: Dave Chinner @ 2015-01-27 3:14 UTC (permalink / raw)
To: xfs; +Cc: iustin
From: Dave Chinner <dchinner@redhat.com>
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 <dchinner@redhat.com>
---
fs/xfs/xfs_ioctl.c | 96 +++++++++++++++++++++++++++++-------------------------
1 file changed, 51 insertions(+), 45 deletions(-)
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index ba98412..d06f596 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 futher 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,29 +1140,9 @@ 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))
+ return PTR_ERR(tp);
/*
* Do a quota reservation only if projid is actually going to change.
@@ -1244,20 +1265,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.
@@ -1272,8 +1280,6 @@ xfs_ioctl_setattr(
xfs_qm_dqrele(udqp);
xfs_qm_dqrele(pdqp);
xfs_trans_cancel(tp, 0);
- if (lock_flags)
- xfs_iunlock(ip, lock_flags);
return code;
}
--
2.0.0
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 4/9] xfs: disaggregate xfs_ioctl_setattr
2015-01-27 3:14 [PATCH 0/9] xfs: xfs_ioctl_setxattr rework Dave Chinner
` (2 preceding siblings ...)
2015-01-27 3:14 ` [PATCH 3/9] xfs: factor out xfs_ioctl_setattr transaciton preamble Dave Chinner
@ 2015-01-27 3:14 ` Dave Chinner
2015-01-29 15:34 ` Brian Foster
2015-01-27 3:14 ` [PATCH 5/9] xfs: kill xfs_ioctl_setattr behaviour mask Dave Chinner
` (5 subsequent siblings)
9 siblings, 1 reply; 23+ messages in thread
From: Dave Chinner @ 2015-01-27 3:14 UTC (permalink / raw)
To: xfs; +Cc: iustin
From: Dave Chinner <dchinner@redhat.com>
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 <dchinner@redhat.com>
---
fs/xfs/xfs_ioctl.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index d06f596..c71f32d 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1321,14 +1321,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;
@@ -1338,13 +1338,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;
}
--
2.0.0
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 5/9] xfs: kill xfs_ioctl_setattr behaviour mask
2015-01-27 3:14 [PATCH 0/9] xfs: xfs_ioctl_setxattr rework Dave Chinner
` (3 preceding siblings ...)
2015-01-27 3:14 ` [PATCH 4/9] xfs: disaggregate xfs_ioctl_setattr Dave Chinner
@ 2015-01-27 3:14 ` Dave Chinner
2015-01-29 15:35 ` Brian Foster
2015-01-27 3:14 ` [PATCH 6/9] xfs: XFS_IOCTL_SETXATTR can run in user namespaces Dave Chinner
` (4 subsequent siblings)
9 siblings, 1 reply; 23+ messages in thread
From: Dave Chinner @ 2015-01-27 3:14 UTC (permalink / raw)
To: xfs; +Cc: iustin
From: Dave Chinner <dchinner@redhat.com>
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 <dchinner@redhat.com>
---
fs/xfs/xfs_ioctl.c | 168 +++++++++++++++++++++--------------------------------
1 file changed, 65 insertions(+), 103 deletions(-)
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index c71f32d..563d2b4 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);
@@ -1149,72 +1144,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;
}
@@ -1223,32 +1199,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);
}
/*
@@ -1256,14 +1225,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);
@@ -1290,18 +1255,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;
}
--
2.0.0
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 6/9] xfs: XFS_IOCTL_SETXATTR can run in user namespaces
2015-01-27 3:14 [PATCH 0/9] xfs: xfs_ioctl_setxattr rework Dave Chinner
` (4 preceding siblings ...)
2015-01-27 3:14 ` [PATCH 5/9] xfs: kill xfs_ioctl_setattr behaviour mask Dave Chinner
@ 2015-01-27 3:14 ` Dave Chinner
2015-01-29 15:35 ` Brian Foster
2015-01-27 3:14 ` [PATCH 7/9] xfs; factor extsize hint checking out of xfs_ioctl_setattr Dave Chinner
` (3 subsequent siblings)
9 siblings, 1 reply; 23+ messages in thread
From: Dave Chinner @ 2015-01-27 3:14 UTC (permalink / raw)
To: xfs; +Cc: iustin
From: Dave Chinner <dchinner@redhat.com>
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 <dchinner@redhat.com>
---
fs/xfs/xfs_ioctl.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 563d2b4..ae6e1e3 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1120,6 +1120,19 @@ xfs_ioctl_setattr(
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
* is messy. We don't care to take a readlock to look at the ids
@@ -1139,15 +1152,6 @@ xfs_ioctl_setattr(
if (IS_ERR(tp))
return PTR_ERR(tp);
- /*
- * 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) {
--
2.0.0
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 7/9] xfs; factor extsize hint checking out of xfs_ioctl_setattr
2015-01-27 3:14 [PATCH 0/9] xfs: xfs_ioctl_setxattr rework Dave Chinner
` (5 preceding siblings ...)
2015-01-27 3:14 ` [PATCH 6/9] xfs: XFS_IOCTL_SETXATTR can run in user namespaces Dave Chinner
@ 2015-01-27 3:14 ` Dave Chinner
2015-01-29 15:35 ` Brian Foster
2015-01-27 3:14 ` [PATCH 8/9] xfs; factor projid " Dave Chinner
` (2 subsequent siblings)
9 siblings, 1 reply; 23+ messages in thread
From: Dave Chinner @ 2015-01-27 3:14 UTC (permalink / raw)
To: xfs; +Cc: iustin
From: Dave Chinner <dchinner@redhat.com>
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 <dchinner@redhat.com>
---
fs/xfs/xfs_ioctl.c | 82 +++++++++++++++++++++++++++++++-----------------------
1 file changed, 47 insertions(+), 35 deletions(-)
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index ae6e1e3..7e42d0f 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,
@@ -1161,43 +1206,10 @@ xfs_ioctl_setattr(
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))
+ code = xfs_ioctl_setattr_check_extsize(ip, fa);
+ if (code)
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;
- }
-
-
code = xfs_ioctl_setattr_xflags(tp, ip, fa);
if (code)
goto error_return;
--
2.0.0
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 8/9] xfs; factor projid hint checking out of xfs_ioctl_setattr
2015-01-27 3:14 [PATCH 0/9] xfs: xfs_ioctl_setxattr rework Dave Chinner
` (6 preceding siblings ...)
2015-01-27 3:14 ` [PATCH 7/9] xfs; factor extsize hint checking out of xfs_ioctl_setattr Dave Chinner
@ 2015-01-27 3:14 ` Dave Chinner
2015-01-29 15:35 ` Brian Foster
2015-01-27 3:14 ` [PATCH 9/9] xfs: fix behaviour of XFS_IOC_FSSETXATTR on directories Dave Chinner
2015-01-29 15:38 ` [PATCH 0/9] xfs: xfs_ioctl_setxattr rework Brian Foster
9 siblings, 1 reply; 23+ messages in thread
From: Dave Chinner @ 2015-01-27 3:14 UTC (permalink / raw)
To: xfs; +Cc: iustin
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 <dchinner@redhat.com>
---
fs/xfs/xfs_ioctl.c | 50 +++++++++++++++++++++++++++++++-------------------
1 file changed, 31 insertions(+), 19 deletions(-)
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 7e42d0f..561d142 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,
--
2.0.0
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 9/9] xfs: fix behaviour of XFS_IOC_FSSETXATTR on directories
2015-01-27 3:14 [PATCH 0/9] xfs: xfs_ioctl_setxattr rework Dave Chinner
` (7 preceding siblings ...)
2015-01-27 3:14 ` [PATCH 8/9] xfs; factor projid " Dave Chinner
@ 2015-01-27 3:14 ` Dave Chinner
2015-01-29 15:35 ` Brian Foster
2015-01-29 15:38 ` [PATCH 0/9] xfs: xfs_ioctl_setxattr rework Brian Foster
9 siblings, 1 reply; 23+ messages in thread
From: Dave Chinner @ 2015-01-27 3:14 UTC (permalink / raw)
To: xfs; +Cc: iustin
From: Iustin Pop <iustin@k1024.org>
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 <iustin@k1024.org>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
fs/xfs/xfs_ioctl.c | 39 +++++++++++++++++++++++++--------------
1 file changed, 25 insertions(+), 14 deletions(-)
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 561d142..5d9dc69 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,
--
2.0.0
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 1/9] xfs: FSX_NONBLOCK is not used
2015-01-27 3:14 ` [PATCH 1/9] xfs: FSX_NONBLOCK is not used Dave Chinner
@ 2015-01-29 15:33 ` Brian Foster
0 siblings, 0 replies; 23+ messages in thread
From: Brian Foster @ 2015-01-29 15:33 UTC (permalink / raw)
To: Dave Chinner; +Cc: iustin, xfs
On Tue, Jan 27, 2015 at 02:14:38PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> 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 <dchinner@redhat.com>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/xfs_ioctl.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 8810959..09325ff 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);
> --
> 2.0.0
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/9] xfs: separate xflags from xfs_ioctl_setattr
2015-01-27 3:14 ` [PATCH 2/9] xfs: separate xflags from xfs_ioctl_setattr Dave Chinner
@ 2015-01-29 15:34 ` Brian Foster
0 siblings, 0 replies; 23+ messages in thread
From: Brian Foster @ 2015-01-29 15:34 UTC (permalink / raw)
To: Dave Chinner; +Cc: iustin, xfs
On Tue, Jan 27, 2015 at 02:14:39PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> 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 <dchinner@redhat.com>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/xfs_ioctl.c | 89 +++++++++++++++++++++++++-----------------------------
> 1 file changed, 41 insertions(+), 48 deletions(-)
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 09325ff..ba98412 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.
> --
> 2.0.0
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/9] xfs: factor out xfs_ioctl_setattr transaciton preamble
2015-01-27 3:14 ` [PATCH 3/9] xfs: factor out xfs_ioctl_setattr transaciton preamble Dave Chinner
@ 2015-01-29 15:34 ` Brian Foster
0 siblings, 0 replies; 23+ messages in thread
From: Brian Foster @ 2015-01-29 15:34 UTC (permalink / raw)
To: Dave Chinner; +Cc: iustin, xfs
On Tue, Jan 27, 2015 at 02:14:40PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> 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 <dchinner@redhat.com>
> ---
> fs/xfs/xfs_ioctl.c | 96 +++++++++++++++++++++++++++++-------------------------
> 1 file changed, 51 insertions(+), 45 deletions(-)
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index ba98412..d06f596 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 futher operation specific checks. On
further
> + * 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,29 +1140,9 @@ 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))
> + return PTR_ERR(tp);
>
We need to handle udqp and pdqp in this failure case. The rest looks Ok
to me.
Brian
> /*
> * Do a quota reservation only if projid is actually going to change.
> @@ -1244,20 +1265,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.
> @@ -1272,8 +1280,6 @@ xfs_ioctl_setattr(
> xfs_qm_dqrele(udqp);
> xfs_qm_dqrele(pdqp);
> xfs_trans_cancel(tp, 0);
> - if (lock_flags)
> - xfs_iunlock(ip, lock_flags);
> return code;
> }
>
> --
> 2.0.0
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/9] xfs: disaggregate xfs_ioctl_setattr
2015-01-27 3:14 ` [PATCH 4/9] xfs: disaggregate xfs_ioctl_setattr Dave Chinner
@ 2015-01-29 15:34 ` Brian Foster
0 siblings, 0 replies; 23+ messages in thread
From: Brian Foster @ 2015-01-29 15:34 UTC (permalink / raw)
To: Dave Chinner; +Cc: iustin, xfs
On Tue, Jan 27, 2015 at 02:14:41PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> 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 <dchinner@redhat.com>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/xfs_ioctl.c | 23 ++++++++++++++++++-----
> 1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index d06f596..c71f32d 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1321,14 +1321,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;
> @@ -1338,13 +1338,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;
> }
> --
> 2.0.0
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/9] xfs: kill xfs_ioctl_setattr behaviour mask
2015-01-27 3:14 ` [PATCH 5/9] xfs: kill xfs_ioctl_setattr behaviour mask Dave Chinner
@ 2015-01-29 15:35 ` Brian Foster
0 siblings, 0 replies; 23+ messages in thread
From: Brian Foster @ 2015-01-29 15:35 UTC (permalink / raw)
To: Dave Chinner; +Cc: iustin, xfs
On Tue, Jan 27, 2015 at 02:14:42PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> 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 <dchinner@redhat.com>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/xfs_ioctl.c | 168 +++++++++++++++++++++--------------------------------
> 1 file changed, 65 insertions(+), 103 deletions(-)
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index c71f32d..563d2b4 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);
> @@ -1149,72 +1144,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;
> }
>
>
> @@ -1223,32 +1199,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);
> }
>
> /*
> @@ -1256,14 +1225,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);
>
> @@ -1290,18 +1255,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;
> }
> --
> 2.0.0
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 6/9] xfs: XFS_IOCTL_SETXATTR can run in user namespaces
2015-01-27 3:14 ` [PATCH 6/9] xfs: XFS_IOCTL_SETXATTR can run in user namespaces Dave Chinner
@ 2015-01-29 15:35 ` Brian Foster
2015-01-29 23:53 ` Dave Chinner
0 siblings, 1 reply; 23+ messages in thread
From: Brian Foster @ 2015-01-29 15:35 UTC (permalink / raw)
To: Dave Chinner; +Cc: iustin, xfs
On Tue, Jan 27, 2015 at 02:14:43PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> 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 <dchinner@redhat.com>
> ---
> fs/xfs/xfs_ioctl.c | 22 +++++++++++++---------
> 1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 563d2b4..ae6e1e3 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1120,6 +1120,19 @@ xfs_ioctl_setattr(
> 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))
Why not use != here? Looks fine, anyways:
Reviewed-by: Brian Foster <bfoster@redhat.com>
> + 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
> * is messy. We don't care to take a readlock to look at the ids
> @@ -1139,15 +1152,6 @@ xfs_ioctl_setattr(
> if (IS_ERR(tp))
> return PTR_ERR(tp);
>
> - /*
> - * 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) {
> --
> 2.0.0
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 7/9] xfs; factor extsize hint checking out of xfs_ioctl_setattr
2015-01-27 3:14 ` [PATCH 7/9] xfs; factor extsize hint checking out of xfs_ioctl_setattr Dave Chinner
@ 2015-01-29 15:35 ` Brian Foster
0 siblings, 0 replies; 23+ messages in thread
From: Brian Foster @ 2015-01-29 15:35 UTC (permalink / raw)
To: Dave Chinner; +Cc: iustin, xfs
On Tue, Jan 27, 2015 at 02:14:44PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> 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 <dchinner@redhat.com>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/xfs_ioctl.c | 82 +++++++++++++++++++++++++++++++-----------------------
> 1 file changed, 47 insertions(+), 35 deletions(-)
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index ae6e1e3..7e42d0f 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,
> @@ -1161,43 +1206,10 @@ xfs_ioctl_setattr(
> 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))
> + code = xfs_ioctl_setattr_check_extsize(ip, fa);
> + if (code)
> 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;
> - }
> -
> -
> code = xfs_ioctl_setattr_xflags(tp, ip, fa);
> if (code)
> goto error_return;
> --
> 2.0.0
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 8/9] xfs; factor projid hint checking out of xfs_ioctl_setattr
2015-01-27 3:14 ` [PATCH 8/9] xfs; factor projid " Dave Chinner
@ 2015-01-29 15:35 ` Brian Foster
0 siblings, 0 replies; 23+ messages in thread
From: Brian Foster @ 2015-01-29 15:35 UTC (permalink / raw)
To: Dave Chinner; +Cc: iustin, xfs
On Tue, Jan 27, 2015 at 02:14:45PM +1100, Dave Chinner wrote:
> 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 <dchinner@redhat.com>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/xfs_ioctl.c | 50 +++++++++++++++++++++++++++++++-------------------
> 1 file changed, 31 insertions(+), 19 deletions(-)
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 7e42d0f..561d142 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,
> --
> 2.0.0
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 9/9] xfs: fix behaviour of XFS_IOC_FSSETXATTR on directories
2015-01-27 3:14 ` [PATCH 9/9] xfs: fix behaviour of XFS_IOC_FSSETXATTR on directories Dave Chinner
@ 2015-01-29 15:35 ` Brian Foster
0 siblings, 0 replies; 23+ messages in thread
From: Brian Foster @ 2015-01-29 15:35 UTC (permalink / raw)
To: Dave Chinner; +Cc: iustin, xfs
On Tue, Jan 27, 2015 at 02:14:46PM +1100, Dave Chinner wrote:
> From: Iustin Pop <iustin@k1024.org>
>
> 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 <iustin@k1024.org>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/xfs_ioctl.c | 39 +++++++++++++++++++++++++--------------
> 1 file changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 561d142..5d9dc69 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,
> --
> 2.0.0
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/9] xfs: xfs_ioctl_setxattr rework
2015-01-27 3:14 [PATCH 0/9] xfs: xfs_ioctl_setxattr rework Dave Chinner
` (8 preceding siblings ...)
2015-01-27 3:14 ` [PATCH 9/9] xfs: fix behaviour of XFS_IOC_FSSETXATTR on directories Dave Chinner
@ 2015-01-29 15:38 ` Brian Foster
9 siblings, 0 replies; 23+ messages in thread
From: Brian Foster @ 2015-01-29 15:38 UTC (permalink / raw)
To: Dave Chinner; +Cc: iustin, xfs
On Tue, Jan 27, 2015 at 02:14:37PM +1100, Dave Chinner wrote:
> Hi folks,
>
> This is a series I started a few months ago when we first started
> talking about the issues with extent size hints on directories and
> the project ID inherit flags being set on regular files. The code
> is particularly nice and has no clear definition of what sort
> of changes we allow in the XFS_IOC_FSSETXATTR ioctl.
>
> The first thing the series does is kill the FSX_* flags and separate
> out the two different use cases for the xfs_ioctl_setattr()
> function. The first is just changing a constrained set of flags via
> the xfs_ioc_setxflags(), and the second is supporting
> xfs_ioc_fssetxattr(). Factoring out the part of the code that sets
> just the inode flags appropriately allows us to kill the FSX_* flags
> completely.
>
> The next patch then relaxes the overly defensive approach to
> restricting XFS_IOC_FSSETXATTR to only the init namespace. We really
> only need to restrict project ID changes - allowing changes to other
> parts of the inode are managed by user/group permissions which are
> already user namespace aware.
>
> The next part of the patch set factors out the validity checking
> of extent size changes and project ID changes from the setattr
> functions, making it much clearer the separation between checks and
> actions performed by xfs_ioctl_setattr() function.
>
> Finally, with all these changes, Iustin Pop's extent size change
> validity checking patch is ported on top. That now becomes a simple,
> obvious set of changes to an isolated function, and i've added
> comments to explain the rules allowing extent size hints to be
> changed.
>
> Comments, thoughts, flames, etc all welcome.
>
All in all this looks like a nice cleanup to me. By the way, you've got
"xfs; ..." in some of the patch/mail headers instead of "xfs: ..."
Brian
> - Dave.
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 6/9] xfs: XFS_IOCTL_SETXATTR can run in user namespaces
2015-01-29 15:35 ` Brian Foster
@ 2015-01-29 23:53 ` Dave Chinner
2015-01-30 3:04 ` Brian Foster
0 siblings, 1 reply; 23+ messages in thread
From: Dave Chinner @ 2015-01-29 23:53 UTC (permalink / raw)
To: Brian Foster; +Cc: iustin, xfs
On Thu, Jan 29, 2015 at 10:35:15AM -0500, Brian Foster wrote:
> On Tue, Jan 27, 2015 at 02:14:43PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > 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 <dchinner@redhat.com>
> > ---
> > fs/xfs/xfs_ioctl.c | 22 +++++++++++++---------
> > 1 file changed, 13 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 563d2b4..ae6e1e3 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -1120,6 +1120,19 @@ xfs_ioctl_setattr(
> > 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))
>
> Why not use != here? Looks fine, anyways:
Because ^ has an implicit cast of the variables to boolean (i.e flag
set or not), whereas != will only work if XFS_XFLAG_PROJINHERIT =
XFS_DIFLAG_PROJINHERIT. Given that the moment we add more DIFLAGs to
the xfs inode, the current "XFLAG value must match DIFLAG value"
rule is going to be broken, I think that logical evaluation is a
much safer practice for these types of comparisons.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 6/9] xfs: XFS_IOCTL_SETXATTR can run in user namespaces
2015-01-29 23:53 ` Dave Chinner
@ 2015-01-30 3:04 ` Brian Foster
2015-01-30 7:44 ` Dave Chinner
0 siblings, 1 reply; 23+ messages in thread
From: Brian Foster @ 2015-01-30 3:04 UTC (permalink / raw)
To: Dave Chinner; +Cc: iustin, xfs
On Fri, Jan 30, 2015 at 10:53:15AM +1100, Dave Chinner wrote:
> On Thu, Jan 29, 2015 at 10:35:15AM -0500, Brian Foster wrote:
> > On Tue, Jan 27, 2015 at 02:14:43PM +1100, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > >
> > > 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 <dchinner@redhat.com>
> > > ---
> > > fs/xfs/xfs_ioctl.c | 22 +++++++++++++---------
> > > 1 file changed, 13 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > > index 563d2b4..ae6e1e3 100644
> > > --- a/fs/xfs/xfs_ioctl.c
> > > +++ b/fs/xfs/xfs_ioctl.c
> > > @@ -1120,6 +1120,19 @@ xfs_ioctl_setattr(
> > > 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))
> >
> > Why not use != here? Looks fine, anyways:
>
> Because ^ has an implicit cast of the variables to boolean (i.e flag
> set or not), whereas != will only work if XFS_XFLAG_PROJINHERIT =
> XFS_DIFLAG_PROJINHERIT. Given that the moment we add more DIFLAGs to
> the xfs inode, the current "XFLAG value must match DIFLAG value"
> rule is going to be broken, I think that logical evaluation is a
> much safer practice for these types of comparisons.
>
Hrm, I'm not following how a boolean cast occurs here. Isn't ^ a bitwise
operation? If I tweak the DIFLAG_PROJINHERIT to bit 15 (rather than 9)
and regenerate assembly for the code above, I end up with something like
this:
/* -EINVAL */
movl $-22, %eax #, D.54727
...
/* load up the flags vars */
movzwl 354(%rdi), %ecx # ip_5(D)->i_d.di_flags,ip_5(D)->i_d.di_flags
movl (%rsi), %edx # fa_3(D)->fsx_xflags, D.54728
...
/* grab the associated bits and cmp */
andl $512, %edx #, D.54728
andl $32768, %ecx #, D.54728
cmpl %edx, %ecx # D.54728, D.54728
/* ret 0 if values are equal */
movl $0, %edx #, tmp99
cmove %edx, %eax # tmp99,, D.54727
ret
So it seems like this breaks just the same if the bit meaning doesn't
match between fields..?
Brian
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 6/9] xfs: XFS_IOCTL_SETXATTR can run in user namespaces
2015-01-30 3:04 ` Brian Foster
@ 2015-01-30 7:44 ` Dave Chinner
0 siblings, 0 replies; 23+ messages in thread
From: Dave Chinner @ 2015-01-30 7:44 UTC (permalink / raw)
To: Brian Foster; +Cc: iustin, xfs
On Thu, Jan 29, 2015 at 10:04:11PM -0500, Brian Foster wrote:
> On Fri, Jan 30, 2015 at 10:53:15AM +1100, Dave Chinner wrote:
> > On Thu, Jan 29, 2015 at 10:35:15AM -0500, Brian Foster wrote:
> > > Why not use != here? Looks fine, anyways:
> >
> > Because ^ has an implicit cast of the variables to boolean (i.e flag
> > set or not), whereas != will only work if XFS_XFLAG_PROJINHERIT =
> > XFS_DIFLAG_PROJINHERIT. Given that the moment we add more DIFLAGs to
> > the xfs inode, the current "XFLAG value must match DIFLAG value"
> > rule is going to be broken, I think that logical evaluation is a
> > much safer practice for these types of comparisons.
> >
>
> Hrm, I'm not following how a boolean cast occurs here. Isn't ^ a bitwise
> operation?
Ah, yes, you are right. I'mi not sure what type of crack I was
smoking this morning (or when I wrote it). I'll fix it up....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2015-01-30 7:44 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-27 3:14 [PATCH 0/9] xfs: xfs_ioctl_setxattr rework Dave Chinner
2015-01-27 3:14 ` [PATCH 1/9] xfs: FSX_NONBLOCK is not used Dave Chinner
2015-01-29 15:33 ` Brian Foster
2015-01-27 3:14 ` [PATCH 2/9] xfs: separate xflags from xfs_ioctl_setattr Dave Chinner
2015-01-29 15:34 ` Brian Foster
2015-01-27 3:14 ` [PATCH 3/9] xfs: factor out xfs_ioctl_setattr transaciton preamble Dave Chinner
2015-01-29 15:34 ` Brian Foster
2015-01-27 3:14 ` [PATCH 4/9] xfs: disaggregate xfs_ioctl_setattr Dave Chinner
2015-01-29 15:34 ` Brian Foster
2015-01-27 3:14 ` [PATCH 5/9] xfs: kill xfs_ioctl_setattr behaviour mask Dave Chinner
2015-01-29 15:35 ` Brian Foster
2015-01-27 3:14 ` [PATCH 6/9] xfs: XFS_IOCTL_SETXATTR can run in user namespaces Dave Chinner
2015-01-29 15:35 ` Brian Foster
2015-01-29 23:53 ` Dave Chinner
2015-01-30 3:04 ` Brian Foster
2015-01-30 7:44 ` Dave Chinner
2015-01-27 3:14 ` [PATCH 7/9] xfs; factor extsize hint checking out of xfs_ioctl_setattr Dave Chinner
2015-01-29 15:35 ` Brian Foster
2015-01-27 3:14 ` [PATCH 8/9] xfs; factor projid " Dave Chinner
2015-01-29 15:35 ` Brian Foster
2015-01-27 3:14 ` [PATCH 9/9] xfs: fix behaviour of XFS_IOC_FSSETXATTR on directories Dave Chinner
2015-01-29 15:35 ` Brian Foster
2015-01-29 15:38 ` [PATCH 0/9] xfs: xfs_ioctl_setxattr rework Brian Foster
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.