userns: Convert xfs to use kuid/kgid where appropriate

Use uint32 from init_user_ns for xfs internal uid/gid representation in acl, xfs_icdinode.

From my review of the code earlier that just isn't safe. It allows all

Conversion of kuid/gid is done at the vfs boundary,
other user visible xfs specific interfaces (bulkstat, eofblocks filter) expect uint32 init_user_ns uid/gid values.

From my earlier review of the code conversion at the vfs boundary is

Signed-off-by Dwight Engen <[email protected]>

---
 fs/xfs/xfs_acl.c      | 24 ++++++++++++++++++++----
 fs/xfs/xfs_fs.h       |  4 ++--
 fs/xfs/xfs_icache.c   |  2 +-
 fs/xfs/xfs_inode.c    |  6 +++---
 fs/xfs/xfs_ioctl.c    |  2 +-
 fs/xfs/xfs_iops.c     | 38 ++++++++++++++++++++------------------
 fs/xfs/xfs_qm.c       | 16 ++++++++--------
 fs/xfs/xfs_quota.h    |  9 +++++----
 fs/xfs/xfs_symlink.c  |  4 +++-
 fs/xfs/xfs_vnodeops.c |  4 +++-
 init/Kconfig          | 13 -------------
 11 files changed, 66 insertions(+), 56 deletions(-)

fs/xfs/xfs_acl.c +20 -4

index 306d883..fd2854e 100644
@@ -68,14 +68,17 @@ xfs_acl_from_disk(
 
 		switch (acl_e->e_tag) {
 		case ACL_USER:
+			acl_e->e_uid = make_kuid(&init_user_ns,
+						 be32_to_cpu(ace->ae_id));
+			break;
 		case ACL_GROUP:
-			acl_e->e_id = be32_to_cpu(ace->ae_id);
+			acl_e->e_gid = make_kgid(&init_user_ns,
+						 be32_to_cpu(ace->ae_id));
 			break;
 		case ACL_USER_OBJ:
 		case ACL_GROUP_OBJ:
 		case ACL_MASK:
 		case ACL_OTHER:
-			acl_e->e_id = ACL_UNDEFINED_ID;
 			break;
 		default:
 			goto fail;

@@ -101,7 +104,20 @@ xfs_acl_to_disk(struct xfs_acl *aclp, const struct posix_acl *acl)
 		acl_e = &acl->a_entries[i];
 
 		ace->ae_tag = cpu_to_be32(acl_e->e_tag);
-		ace->ae_id = cpu_to_be32(acl_e->e_id);
+		switch (acl_e->e_tag) {
+		case ACL_USER:
+			ace->ae_id = cpu_to_be32(
+				from_kuid(&init_user_ns, acl_e->e_uid));
+			break;
+		case ACL_GROUP:
+			ace->ae_id = cpu_to_be32(
+				from_kgid(&init_user_ns, acl_e->e_gid));
+			break;
+		default:
+			ace->ae_id = cpu_to_be32(ACL_UNDEFINED_ID);
+			break;
+		}
+
 		ace->ae_perm = cpu_to_be16(acl_e->e_perm);
 	}
 }

@@ -360,7 +376,7 @@ xfs_xattr_acl_set(struct dentry *dentry, const char *name,
 		return -EINVAL;
 	if (type == ACL_TYPE_DEFAULT && !S_ISDIR(inode->i_mode))
 		return value ? -EACCES : 0;
-	if ((current_fsuid() != inode->i_uid) && !capable(CAP_FOWNER))
+	if (!inode_owner_or_capable(inode))
 		return -EPERM;
 
 	if (!value)

fs/xfs/xfs_fs.h +2 -2

index d046955..bf0a6f8 100644
@@ -347,8 +347,8 @@ typedef struct xfs_error_injection {
 struct xfs_eofblocks {
 	__u32		eof_version;
 	__u32		eof_flags;
-	uid_t		eof_uid;
-	gid_t		eof_gid;
+	__u32		eof_uid;
+	__u32		eof_gid;
 	prid_t		eof_prid;
 	__u32		pad32;
 	__u64		eof_min_file_size;

fs/xfs/xfs_icache.c +1 -1

index 96e344e..70ba410 100644
@@ -617,7 +617,7 @@ restart:
 
 /*
  * Background scanning to trim post-EOF preallocated space. This is queued
- * based on the 'background_prealloc_discard_period' tunable (5m by default).
+ * based on the 'speculative_prealloc_lifetime' tunable (5m by default).
  */
 STATIC void
 xfs_queue_eofblocks(

fs/xfs/xfs_inode.c +3 -3

index 7f7be5f..8049976 100644
@@ -1268,8 +1268,8 @@ xfs_ialloc(
 	ip->i_d.di_onlink = 0;
 	ip->i_d.di_nlink = nlink;
 	ASSERT(ip->i_d.di_nlink == nlink);
-	ip->i_d.di_uid = current_fsuid();
-	ip->i_d.di_gid = current_fsgid();
+	ip->i_d.di_uid = from_kuid(&init_user_ns, current_fsuid());
+	ip->i_d.di_gid = from_kgid(&init_user_ns, current_fsgid());
 	xfs_set_projid(ip, prid);
 	memset(&(ip->i_d.di_pad[0]), 0, sizeof(ip->i_d.di_pad));
 

@@ -1308,7 +1308,7 @@ xfs_ialloc(
 	 */
 	if ((irix_sgid_inherit) &&
 	    (ip->i_d.di_mode & S_ISGID) &&
-	    (!in_group_p((gid_t)ip->i_d.di_gid))) {
+	    (!in_group_p(make_kgid(&init_user_ns, ip->i_d.di_gid)))) {
 		ip->i_d.di_mode &= ~S_ISGID;
 	}
 

fs/xfs/xfs_ioctl.c +1 -1

index 5e99968..daa6127 100644
@@ -981,7 +981,7 @@ xfs_ioctl_setattr(
 	 * to the file owner ID, except in cases where the
 	 * CAP_FSETID capability is applicable.
 	 */
-	if (current_fsuid() != ip->i_d.di_uid && !capable(CAP_FOWNER)) {
+	if (!inode_owner_or_capable(&ip->i_vnode)) {
 		code = XFS_ERROR(EPERM);
 		goto error_return;
 	}

fs/xfs/xfs_iops.c +20 -18

index ca9ecaa..bf96cf8 100644
@@ -420,8 +420,8 @@ xfs_vn_getattr(
 	stat->dev = inode->i_sb->s_dev;
 	stat->mode = ip->i_d.di_mode;
 	stat->nlink = ip->i_d.di_nlink;
-	stat->uid = ip->i_d.di_uid;
-	stat->gid = ip->i_d.di_gid;
+	stat->uid = make_kuid(&init_user_ns, ip->i_d.di_uid);
+	stat->gid = make_kgid(&init_user_ns, ip->i_d.di_gid);
 	stat->ino = ip->i_ino;
 	stat->atime = inode->i_atime;
 	stat->mtime = inode->i_mtime;

@@ -488,8 +488,8 @@ xfs_setattr_nonsize(
 	int			mask = iattr->ia_valid;
 	xfs_trans_t		*tp;
 	int			error;
-	uid_t			uid = 0, iuid = 0;
-	gid_t			gid = 0, igid = 0;
+	kuid_t			uid = GLOBAL_ROOT_UID, iuid = GLOBAL_ROOT_UID;
+	kgid_t			gid = GLOBAL_ROOT_GID, igid = GLOBAL_ROOT_GID;
 	struct xfs_dquot	*udqp = NULL, *gdqp = NULL;
 	struct xfs_dquot	*olddquot1 = NULL, *olddquot2 = NULL;
 

@@ -522,13 +522,13 @@ xfs_setattr_nonsize(
 			uid = iattr->ia_uid;
 			qflags |= XFS_QMOPT_UQUOTA;
 		} else {
-			uid = ip->i_d.di_uid;
+			uid = make_kuid(&init_user_ns, ip->i_d.di_uid);
 		}
 		if ((mask & ATTR_GID) && XFS_IS_GQUOTA_ON(mp)) {
 			gid = iattr->ia_gid;
 			qflags |= XFS_QMOPT_GQUOTA;
 		}  else {
-			gid = ip->i_d.di_gid;
+			gid = make_kgid(&init_user_ns, ip->i_d.di_gid);
 		}
 
 		/*

@@ -538,8 +538,10 @@ xfs_setattr_nonsize(
 		 */
 		ASSERT(udqp == NULL);
 		ASSERT(gdqp == NULL);
-		error = xfs_qm_vop_dqalloc(ip, uid, gid, xfs_get_projid(ip),
-					 qflags, &udqp, &gdqp);
+		error = xfs_qm_vop_dqalloc(ip, from_kuid(&init_user_ns, uid),
+					   from_kgid(&init_user_ns, gid),
+					   xfs_get_projid(ip),
+					   qflags, &udqp, &gdqp);
 		if (error)
 			return error;
 	}

@@ -561,8 +563,8 @@ xfs_setattr_nonsize(
 		 * while we didn't have the inode locked, inode's dquot(s)
 		 * would have changed also.
 		 */
-		iuid = ip->i_d.di_uid;
-		igid = ip->i_d.di_gid;
+		iuid = make_kuid(&init_user_ns, ip->i_d.di_uid);
+		igid = make_kgid(&init_user_ns, ip->i_d.di_gid);
 		gid = (mask & ATTR_GID) ? iattr->ia_gid : igid;
 		uid = (mask & ATTR_UID) ? iattr->ia_uid : iuid;
 

@@ -571,8 +573,8 @@ xfs_setattr_nonsize(
 		 * going to change.
 		 */
 		if (XFS_IS_QUOTA_RUNNING(mp) &&
-		    ((XFS_IS_UQUOTA_ON(mp) && iuid != uid) ||
-		     (XFS_IS_GQUOTA_ON(mp) && igid != gid))) {
+		    ((XFS_IS_UQUOTA_ON(mp) && !uid_eq(iuid, uid)) ||
+		     (XFS_IS_GQUOTA_ON(mp) && !gid_eq(igid, gid)))) {
 			ASSERT(tp);
 			error = xfs_qm_vop_chown_reserve(tp, ip, udqp, gdqp,
 						capable(CAP_FOWNER) ?

@@ -602,17 +604,17 @@ xfs_setattr_nonsize(
 		 * Change the ownerships and register quota modifications
 		 * in the transaction.
 		 */
-		if (iuid != uid) {
+		if (!uid_eq(iuid, uid)) {
 			if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_UQUOTA_ON(mp)) {
 				ASSERT(mask & ATTR_UID);
 				ASSERT(udqp);
 				olddquot1 = xfs_qm_vop_chown(tp, ip,
 							&ip->i_udquot, udqp);
 			}
-			ip->i_d.di_uid = uid;
+			ip->i_d.di_uid = from_kuid(&init_user_ns, uid);
 			inode->i_uid = uid;
 		}
-		if (igid != gid) {
+		if (!gid_eq(igid, gid)) {
 			if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_GQUOTA_ON(mp)) {
 				ASSERT(!XFS_IS_PQUOTA_ON(mp));
 				ASSERT(mask & ATTR_GID);

@@ -620,7 +622,7 @@ xfs_setattr_nonsize(
 				olddquot2 = xfs_qm_vop_chown(tp, ip,
 							&ip->i_gdquot, gdqp);
 			}
-			ip->i_d.di_gid = gid;
+			ip->i_d.di_gid = from_kgid(&init_user_ns, gid);
 			inode->i_gid = gid;
 		}
 	}

@@ -1172,8 +1174,8 @@ xfs_setup_inode(
 
 	inode->i_mode	= ip->i_d.di_mode;
 	set_nlink(inode, ip->i_d.di_nlink);
-	inode->i_uid	= ip->i_d.di_uid;
-	inode->i_gid	= ip->i_d.di_gid;
+	inode->i_uid	= make_kuid(&init_user_ns, ip->i_d.di_uid);
+	inode->i_gid	= make_kgid(&init_user_ns, ip->i_d.di_gid);
 
 	switch (inode->i_mode & S_IFMT) {
 	case S_IFBLK:

fs/xfs/xfs_qm.c +8 -8

index b75c9bb..94a2a8f 100644
@@ -1651,8 +1651,8 @@ xfs_qm_write_sb_changes(
 int
 xfs_qm_vop_dqalloc(
 	struct xfs_inode	*ip,
-	uid_t			uid,
-	gid_t			gid,
+	__uint32_t		di_uid,
+	__uint32_t		di_gid,
 	prid_t			prid,
 	uint			flags,
 	struct xfs_dquot	**O_udqpp,

@@ -1670,7 +1670,7 @@ xfs_qm_vop_dqalloc(
 	xfs_ilock(ip, lockflags);
 
 	if ((flags & XFS_QMOPT_INHERIT) && XFS_INHERIT_GID(ip))
-		gid = ip->i_d.di_gid;
+		di_gid = ip->i_d.di_gid;
 
 	/*
 	 * Attach the dquot(s) to this inode, doing a dquot allocation

@@ -1686,7 +1686,7 @@ xfs_qm_vop_dqalloc(
 
 	uq = gq = NULL;
 	if ((flags & XFS_QMOPT_UQUOTA) && XFS_IS_UQUOTA_ON(mp)) {
-		if (ip->i_d.di_uid != uid) {
+		if (ip->i_d.di_uid != di_uid) {
 			/*
 			 * What we need is the dquot that has this uid, and
 			 * if we send the inode to dqget, the uid of the inode

@@ -1697,7 +1697,7 @@ xfs_qm_vop_dqalloc(
 			 * holding ilock.
 			 */
 			xfs_iunlock(ip, lockflags);
-			if ((error = xfs_qm_dqget(mp, NULL, (xfs_dqid_t) uid,
+			if ((error = xfs_qm_dqget(mp, NULL, (xfs_dqid_t)di_uid,
 						 XFS_DQ_USER,
 						 XFS_QMOPT_DQALLOC |
 						 XFS_QMOPT_DOWARN,

@@ -1721,9 +1721,9 @@ xfs_qm_vop_dqalloc(
 		}
 	}
 	if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
-		if (ip->i_d.di_gid != gid) {
+		if (ip->i_d.di_gid != di_gid) {
 			xfs_iunlock(ip, lockflags);
-			if ((error = xfs_qm_dqget(mp, NULL, (xfs_dqid_t)gid,
+			if ((error = xfs_qm_dqget(mp, NULL, (xfs_dqid_t)di_gid,
 						 XFS_DQ_GROUP,
 						 XFS_QMOPT_DQALLOC |
 						 XFS_QMOPT_DOWARN,

@@ -1842,7 +1842,7 @@ xfs_qm_vop_chown_reserve(
 			XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS;
 
 	if (XFS_IS_UQUOTA_ON(mp) && udqp &&
-	    ip->i_d.di_uid != (uid_t)be32_to_cpu(udqp->q_core.d_id)) {
+	    ip->i_d.di_uid != be32_to_cpu(udqp->q_core.d_id)) {
 		delblksudq = udqp;
 		/*
 		 * If there are delayed allocation blocks, then we have to

fs/xfs/xfs_quota.h +5 -4

index c38068f..0464d77 100644
@@ -320,8 +320,8 @@ extern int xfs_trans_reserve_quota_bydquots(struct xfs_trans *,
 		struct xfs_mount *, struct xfs_dquot *,
 		struct xfs_dquot *, long, long, uint);
 
-extern int xfs_qm_vop_dqalloc(struct xfs_inode *, uid_t, gid_t, prid_t, uint,
-		struct xfs_dquot **, struct xfs_dquot **);
+extern int xfs_qm_vop_dqalloc(struct xfs_inode *, __uint32_t, __uint32_t,
+		prid_t, uint, struct xfs_dquot **, struct xfs_dquot **);
 extern void xfs_qm_vop_create_dqattach(struct xfs_trans *, struct xfs_inode *,
 		struct xfs_dquot *, struct xfs_dquot *);
 extern int xfs_qm_vop_rename_dqattach(struct xfs_inode **);

@@ -341,8 +341,9 @@ extern void xfs_qm_unmount_quotas(struct xfs_mount *);
 
 #else
 static inline int
-xfs_qm_vop_dqalloc(struct xfs_inode *ip, uid_t uid, gid_t gid, prid_t prid,
-		uint flags, struct xfs_dquot **udqp, struct xfs_dquot **gdqp)
+xfs_qm_vop_dqalloc(struct xfs_inode *ip, __uint32_t uid, __uint32_t gid,
+		prid_t prid, uint flags, struct xfs_dquot **udqp,
+		struct xfs_dquot **gdqp)
 {
 	*udqp = NULL;
 	*gdqp = NULL;

fs/xfs/xfs_symlink.c +3 -1

index 195a403..3f7cfb3 100644
@@ -384,7 +384,9 @@ xfs_symlink(
 	/*
 	 * Make sure that we have allocated dquot(s) on disk.
 	 */
-	error = xfs_qm_vop_dqalloc(dp, current_fsuid(), current_fsgid(), prid,
+	error = xfs_qm_vop_dqalloc(dp,
+			from_kuid(&init_user_ns, current_fsuid()),
+			from_kgid(&init_user_ns, current_fsgid()), prid,
 			XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT, &udqp, &gdqp);
 	if (error)
 		goto std_return;

fs/xfs/xfs_vnodeops.c +3 -1

index 0176bb2..37e9d4a 100644
@@ -515,7 +515,9 @@ xfs_create(
 	/*
 	 * Make sure that we have allocated dquot(s) on disk.
 	 */
-	error = xfs_qm_vop_dqalloc(dp, current_fsuid(), current_fsgid(), prid,
+	error = xfs_qm_vop_dqalloc(dp,
+			from_kuid(&init_user_ns, current_fsuid()),
+			from_kgid(&init_user_ns, current_fsgid()), prid,
 			XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT, &udqp, &gdqp);
 	if (error)
 		return error;

init/Kconfig +0 -8

index 9d3a788..fe29801 100644
@@ -1065,7 +1065,6 @@ config IPC_NS
 
 config USER_NS
 	bool "User namespace"
-	depends on UIDGID_CONVERTED
 	select UIDGID_STRICT_TYPE_CHECKS
 
 	default n

@@ -1099,20 +1098,8 @@ config NET_NS
 
 endif # NAMESPACES
 
-config UIDGID_CONVERTED
-	# True if all of the selected software conmponents are known
-	# to have uid_t and gid_t converted to kuid_t and kgid_t
-	# where appropriate and are otherwise safe to use with
-	# the user namespace.
-	bool
-	default y

Use uint32 from init_user_ns for xfs internal uid/gid representation in acl, xfs_icdinode. Conversion of kuid/gid is done at the vfs boundary, other user visible xfs specific interfaces (bulkstat, eofblocks filter) expect uint32 init_user_ns uid/gid values.

Signed-off-by Dwight Engen <[email protected]>

fs/xfs/xfs_icache.c +1 -1

index 96e344e..70ba410 100644
@@ -617,7 +617,7 @@ restart:
 
 /*
  * Background scanning to trim post-EOF preallocated space. This is queued
- * based on the 'background_prealloc_discard_period' tunable (5m by default).
+ * based on the 'speculative_prealloc_lifetime' tunable (5m by default).
  */
 STATIC void
 xfs_queue_eofblocks(

fs/xfs/xfs_inode.c +0 -0

index 7f7be5f..8049976 100644

fs/xfs/xfs_inode.c +1 -1

 	xfs_set_projid(ip, prid);
 	memset(&(ip->i_d.di_pad[0]), 0, sizeof(ip->i_d.di_pad));
 

@@ -1308,7 +1308,7 @@ xfs_ialloc(
 	 */
 	if ((irix_sgid_inherit) &&
 	    (ip->i_d.di_mode & S_ISGID) &&
-	    (!in_group_p((gid_t)ip->i_d.di_gid))) {
+	    (!in_group_p(make_kgid(&init_user_ns, ip->i_d.di_gid)))) {
 		ip->i_d.di_mode &= ~S_ISGID;
 	}
 

fs/xfs/xfs_ioctl.c +0 -0

index 5e99968..daa6127 100644

fs/xfs/xfs_ioctl.c +0 -0

 		code = XFS_ERROR(EPERM);
 		goto error_return;
 	}

fs/xfs/xfs_iops.c +0 -0

index ca9ecaa..bf96cf8 100644

fs/xfs/xfs_iops.c +2 -2

 	stat->ino = ip->i_ino;
 	stat->atime = inode->i_atime;
 	stat->mtime = inode->i_mtime;

@@ -488,8 +488,8 @@ xfs_setattr_nonsize(
 	int			mask = iattr->ia_valid;
 	xfs_trans_t		*tp;
 	int			error;
-	uid_t			uid = 0, iuid = 0;
-	gid_t			gid = 0, igid = 0;
+	kuid_t			uid = GLOBAL_ROOT_UID, iuid = GLOBAL_ROOT_UID;
+	kgid_t			gid = GLOBAL_ROOT_GID, igid = GLOBAL_ROOT_GID;
 	struct xfs_dquot	*udqp = NULL, *gdqp = NULL;
 	struct xfs_dquot	*olddquot1 = NULL, *olddquot2 = NULL;
 

fs/xfs/xfs_iops.c +0 -0

 		}

fs/xfs/xfs_iops.c +0 -0

 	switch (inode->i_mode & S_IFMT) {
 	case S_IFBLK:

fs/xfs/xfs_qm.c +0 -0

index b75c9bb..94a2a8f 100644

I am copying my gmail address so I have a chance of seeing replies from Dave Chiner. So far the only way I have been able to read his replies has been to read mailling lists. Which has not be conductive to having this code discussed properly. Hopefully copying my gmail address will allow us to have a reasonable and timely conversation.

Quoted 2 lines · Dwight Engen

Use uint32 from init_user_ns for xfs internal uid/gid representation in acl, xfs_icdinode.

From my review of the code earlier that just isn't safe. It allows all kinds of things to slip through.

Quoted 3 lines

Conversion of kuid/gid is done at the vfs boundary,
other user visible xfs specific interfaces (bulkstat, eofblocks filter) expect uint32 init_user_ns uid/gid values.

From my earlier review of the code conversion at the vfs boundary is not safe.

First off kuid_t and kgid_t are not a vfs concepts, they are linux kernel concepts, and xfs is in the linux kernel. What makes this relevant is not all filesystem accesses are through the vfs so all of the necessary conversions for security and a consistent user experience can be had by only performing conversions at the user/kernel boundary.

In particular by being sloppy and not pushing kuid_t/kgid_t further down you did not handle all of the conversions needed at the user/kernel boundary in XFS_IOC_FREE_EOFBLOCKS.

Which can be called by an
unprivileged user.

I honestly don't think avoiding the push down of kuid_t and kgid_t to all of the xfs in-core data structures is safe. Even if the initial patch is safe I expect there will be silent breakage when the next ioctl that bypasses the vfs is added.

Quoted 5 lines · Dwight Engen

Use uint32 from init_user_ns for xfs internal uid/gid representation in acl, xfs_icdinode. Conversion of kuid/gid is done at the vfs boundary, other user visible xfs specific interfaces (bulkstat, eofblocks filter) expect uint32 init_user_ns uid/gid values.

It's minimal, but I'm not sure it's complete. I'll comment on that in response to Eric's comments...

Quoted 1 line

Signed-off-by Dwight Engen <[email protected]>

....

fs/xfs/xfs_fs.h typedef struct xfs_error_injection { · line 354

Quoted 13 lines

fs/xfs/xfs_fs.h +2 -2

@@ -347,8 +347,8 @@ typedef struct xfs_error_injection {
 struct xfs_eofblocks {
 	__u32		eof_version;
 	__u32		eof_flags;
-	uid_t		eof_uid;
-	gid_t		eof_gid;
+	__u32		eof_uid;
+	__u32		eof_gid;
 	prid_t		eof_prid;
 	__u32		pad32;
 	__u64		eof_min_file_size;

The patch doesn't do namespace conversion of these uid/gids, but I'm not sure it should...

Quoted 22 lines

fs/xfs/xfs_icache.c +0 -0

index 96e344e..70ba410 100644
@@ -617,7 +617,7 @@ restart:
 
 /*
  * Background scanning to trim post-EOF preallocated space. This

is queued
- * based on the 'background_prealloc_discard_period' tunable (5m by default).
+ * based on the 'speculative_prealloc_lifetime' tunable (5m by default). */
STATIC void
xfs_queue_eofblocks(

fs/xfs/xfs_inode.c +0 -0

index 7f7be5f..8049976 100644
@@ -1268,8 +1268,8 @@ xfs_ialloc(
 	ip->i_d.di_onlink = 0;
 	ip->i_d.di_nlink = nlink;

fs/xfs/xfs_inode.c xfs_ialloc( · line 1272

Quoted 5 lines

fs/xfs/xfs_inode.c +2 -2

 	ASSERT(ip->i_d.di_nlink == nlink);
-	ip->i_d.di_uid = current_fsuid();
-	ip->i_d.di_gid = current_fsgid();
+	ip->i_d.di_uid = from_kuid(&init_user_ns, current_fsuid());
+	ip->i_d.di_gid = from_kgid(&init_user_ns, current_fsgid());

Why all new inodes be created in the init_user_ns? Shouldn't this be current_user_ns()?

Same question throughout - why do you use init_user_ns for all these UID conversions, when the whole point is to have awareness of different namespaces?

Quoted 17 lines

fs/xfs/xfs_inode.c +1 -1

 	xfs_set_projid(ip, prid);
 	memset(&(ip->i_d.di_pad[0]), 0, sizeof(ip->i_d.di_pad));
 

@@ -1308,7 +1308,7 @@ xfs_ialloc(
 	 */
 	if ((irix_sgid_inherit) &&
 	    (ip->i_d.di_mode & S_ISGID) &&
-	    (!in_group_p((gid_t)ip->i_d.di_gid))) {
+	    (!in_group_p(make_kgid(&init_user_ns,
 ip->i_d.di_gid)))) { ip->i_d.di_mode &= ~S_ISGID;
 	}
 

fs/xfs/xfs_ioctl.c +0 -0

index 5e99968..daa6127 100644

fs/xfs/xfs_ioctl.c xfs_ioctl_setattr( · line 985

Quoted 6 lines

fs/xfs/xfs_ioctl.c +1 -1

@@ -981,7 +981,7 @@ xfs_ioctl_setattr(
 	 * to the file owner ID, except in cases where the
 	 * CAP_FSETID capability is applicable.
 	 */
-	if (current_fsuid() != ip->i_d.di_uid
 && !capable(CAP_FOWNER)) {
+	if (!inode_owner_or_capable(&ip->i_vnode)) {

				    VFS_I(ip)

Quoted 8 lines

fs/xfs/xfs_ioctl.c +0 -0

 		code = XFS_ERROR(EPERM);
 		goto error_return;

fs/xfs/xfs_ioctl.c +0 -0

 	}

fs/xfs/xfs_iops.c +0 -0

index ca9ecaa..bf96cf8 100644

fs/xfs/xfs_iops.c xfs_vn_getattr( · line 424

Quoted 7 lines

fs/xfs/xfs_iops.c +2 -2

@@ -420,8 +420,8 @@ xfs_vn_getattr(
 	stat->dev = inode->i_sb->s_dev;
 	stat->mode = ip->i_d.di_mode;
 	stat->nlink = ip->i_d.di_nlink;
-	stat->uid = ip->i_d.di_uid;
-	stat->gid = ip->i_d.di_gid;
+	stat->uid = make_kuid(&init_user_ns, ip->i_d.di_uid);
+	stat->gid = make_kgid(&init_user_ns, ip->i_d.di_gid);

Why not:

	stat->uid = inode->i_uid;
	stat->gid = inode->i_gid;

Quoted 16 lines

fs/xfs/xfs_iops.c +2 -2

 	stat->ino = ip->i_ino;
 	stat->atime = inode->i_atime;
 	stat->mtime = inode->i_mtime;

@@ -488,8 +488,8 @@ xfs_setattr_nonsize(
 	int			mask = iattr->ia_valid;
 	xfs_trans_t		*tp;
 	int			error;
-	uid_t			uid = 0, iuid = 0;
-	gid_t			gid = 0, igid = 0;
+	kuid_t			uid = GLOBAL_ROOT_UID, iuid
 = GLOBAL_ROOT_UID;
+	kgid_t			gid = GLOBAL_ROOT_GID, igid

= GLOBAL_ROOT_GID; struct xfs_dquot *udqp = NULL, *gdqp =
NULL; struct xfs_dquot *olddquot1 = NULL, *olddquot2 = NULL;

fs/xfs/xfs_iops.c xfs_setattr_nonsize( · line 526

Quoted 6 lines

fs/xfs/xfs_iops.c +1 -1

@@ -522,13 +522,13 @@ xfs_setattr_nonsize(
 			uid = iattr->ia_uid;
 			qflags |= XFS_QMOPT_UQUOTA;
 		} else {
-			uid = ip->i_d.di_uid;
+			uid = make_kuid(&init_user_ns,
 ip->i_d.di_uid);

			uid = VFS_I(ip)->i_uid;

fs/xfs/xfs_iops.c xfs_setattr_nonsize(

Quoted 8 lines

fs/xfs/xfs_iops.c +1 -1

 		}
 		if ((mask & ATTR_GID) && XFS_IS_GQUOTA_ON(mp)) {
 			gid = iattr->ia_gid;
 			qflags |= XFS_QMOPT_GQUOTA;
 		}  else {
-			gid = ip->i_d.di_gid;
+			gid = make_kgid(&init_user_ns,
 ip->i_d.di_gid);

			gid = VFS_I(ip)->i_gid;

Quoted 1 line

fs/xfs/xfs_iops.c +0 -0

 		}

.....

fs/xfs/xfs_iops.c xfs_setattr_nonsize( · line 570

Quoted 11 lines

fs/xfs/xfs_iops.c +2 -2

@@ -561,8 +563,8 @@ xfs_setattr_nonsize(
 		 * while we didn't have the inode locked, inode's
 dquot(s)
 		 * would have changed also.
 		 */
-		iuid = ip->i_d.di_uid;
-		igid = ip->i_d.di_gid;
+		iuid = make_kuid(&init_user_ns, ip->i_d.di_uid);
+		igid = make_kgid(&init_user_ns, ip->i_d.di_gid);
 		gid = (mask & ATTR_GID) ? iattr->ia_gid : igid;
 		uid = (mask & ATTR_UID) ? iattr->ia_uid : iuid;

Same again - you can just use VFS_I(ip)->i_uid/VFS_I(ip)->i_gid here.

Quoted 2 lines

fs/xfs/xfs_iops.c +0 -0

@@ -1172,8 +1174,8 @@ xfs_setup_inode(
 

fs/xfs/xfs_iops.c xfs_setup_inode(

Quoted 8 lines

fs/xfs/xfs_iops.c +2 -2

 	inode->i_mode	= ip->i_d.di_mode;
 	set_nlink(inode, ip->i_d.di_nlink);
-	inode->i_uid	= ip->i_d.di_uid;
-	inode->i_gid	= ip->i_d.di_gid;
+	inode->i_uid	= make_kuid(&init_user_ns,
 ip->i_d.di_uid);
+	inode->i_gid	= make_kgid(&init_user_ns,
 ip->i_d.di_gid);

current name space?

Quoted 8 lines

fs/xfs/xfs_iops.c +0 -0

 	switch (inode->i_mode & S_IFMT) {

fs/xfs/xfs_iops.c +0 -0

 	case S_IFBLK:

fs/xfs/xfs_qm.c +0 -0

index b75c9bb..94a2a8f 100644

fs/xfs/xfs_qm.c xfs_qm_write_sb_changes( · line 1655

Quoted 7 lines

fs/xfs/xfs_qm.c +2 -2

@@ -1651,8 +1651,8 @@ xfs_qm_write_sb_changes(
 int
 xfs_qm_vop_dqalloc(
 	struct xfs_inode	*ip,
-	uid_t			uid,
-	gid_t			gid,
+	__uint32_t		di_uid,
+	__uint32_t		di_gid,

xfs_dqid_t

And there's no need to rename the variables - that just causes unnecessary churn, and the fact it is a dquot ID is documented by the type.

Cheers,

Dave.

Quoted 11 lines · Eric W. Biederman

I am copying my gmail address so I have a chance of seeing replies from Dave Chiner. So far the only way I have been able to read his replies has been to read mailling lists. Which has not be conductive to having this code discussed properly. Hopefully copying my gmail address will allow us to have a reasonable and timely conversation.

Dwight Engen <[email protected]> writes:

Quoted 5 lines

Quoted 2 lines

Use uint32 from init_user_ns for xfs internal uid/gid representation in acl, xfs_icdinode.

From my review of the code earlier that just isn't safe. It allows all kinds of things to slip through.

Such as?

Quoted 6 lines

Quoted 3 lines

Conversion of kuid/gid is done at the vfs boundary, other user visible xfs specific interfaces (bulkstat, eofblocks filter) expect uint32 init_user_ns uid/gid values.

From my earlier review of the code conversion at the vfs boundary is not safe.

So you've claimed.

Quoted 6 lines

First off kuid_t and kgid_t are not a vfs concepts, they are linux kernel concepts, and xfs is in the linux kernel. What makes this relevant is not all filesystem accesses are through the vfs so all of the necessary conversions for security and a consistent user experience can be had by only performing conversions at the user/kernel boundary.

Right. Boundaries and consistent conversion across them are important. They are especially important in filesystems for converting to/from on-disk and kernel-native formats. Blindly pushing kernel structure down as far are you can make them reach ignores important boundaries.

You might want to hit XFS with a big hammer but the right solution is far more nuanced that that.

Quoted 3 lines

In particular by being sloppy and not pushing kuid_t/kgid_t further down you did not handle all of the conversions needed at the user/kernel boundary in XFS_IOC_FREE_EOFBLOCKS.

The kuid_t/kgid_t is actually pushed down this far - it's in the struct inode - the code currently uses the on-disk XFS uid/gid, not the struct inode's kuid_t/kgid_t. That's easily fixable.

Indeed, that's where most of the work needs to be done in XFS - using VFS(ip)->i_uid instead of ip->i_d.di_uid in places where we aren't dealing with reading or modifying the on-disk structures of XFS. Once that is done, we will have driven kuid_t/kgid_t as far down as the in-memory/on-disk format boundary allows, and we'll end up catching all the sorts of problems you are worried about.

Quoted 2 lines

Which can be called by an
unprivileged user.

That's an oversite. Needs fixing.

Quoted 4 lines

I honestly don't think avoiding the push down of kuid_t and kgid_t to all of the xfs in-core data structures is safe. Even if the initial patch is safe I expect there will be silent breakage when the next ioctl that bypasses the vfs is added.

This problem isn't isolated to XFS - ioctls are added to ext4, btrfs, gfs2, etc in every release and they all face the same problems. Hence trying to paint it as an XFS problem is realy missing the mark....

I'd really like to see some regressions tests in xfstests that we can use to confirm that filesystems have implemented namespaces properly (e.g. quota set/get/report tests). That would go a long way to ensuring that people don't inadvertantly. I'm sure you have some test scripts for testing all the changes you made, so sharing them would help us a lot.

[ Hmmmm - the quota netlink warning interface - it just takes a warning for a specific kqid and emits it to all listeners on the netlink interface. There's no namespace awareness there, so what stops quota warning messages from one namespace being heard in all other namespaces? It's not network namespace aware.... ]

As it is, I'm far more concerned about the security problems existing ioctls and interfaces pose for user namespaces. i.e. that CAP_SYS_ADMIN in any namespace can use bulkstat and then the fs/fhandle.c interfaces to find and access any file in the filesystem regardless of namespace restrictions. You can guess filehandles pretty trivially, so you don't need bulkstat and you don't need XFS for this to be a problem....

Further, bulkstat is used by backup utilities, so I think it needs the unmodified uid/gid/prid to be passed out, and the restore program needs a way to push them all back in unmodified. How would you propose we go about doing this. Alternatively could you tell us how a sub-namespace level backup/restore program supposed to handle/detect/avoid being invoked from inside a restricted, non-global namespace?

Cheers,

Dave.

Quoted 12 lines · On Thu, 20 Jun 2013 10:13:41 +1000 Dave Chinner <[email protected]> wrote:

Quoted 5 lines · Dwight Engen

Use uint32 from init_user_ns for xfs internal uid/gid representation in acl, xfs_icdinode. Conversion of kuid/gid is done at the vfs boundary, other user visible xfs specific interfaces (bulkstat, eofblocks filter) expect uint32 init_user_ns uid/gid values.

It's minimal, but I'm not sure it's complete. I'll comment on that in response to Eric's comments...

Quoted 1 line

Signed-off-by Dwight Engen <[email protected]>

....

Quoted 16 lines

fs/xfs/xfs_fs.h typedef struct xfs_error_injection { · line 354

Quoted 13 lines

fs/xfs/xfs_fs.h +2 -2

@@ -347,8 +347,8 @@ typedef struct xfs_error_injection {
 struct xfs_eofblocks {
 	__u32		eof_version;
 	__u32		eof_flags;
-	uid_t		eof_uid;
-	gid_t		eof_gid;
+	__u32		eof_uid;
+	__u32		eof_gid;
 	prid_t		eof_prid;
 	__u32		pad32;
 	__u64		eof_min_file_size;

The patch doesn't do namespace conversion of these uid/gids, but I'm not sure it should...

The ids are only advisory, if the caller doesn't specify XFS_EOF_FLAGS_?ID blocks from any inode in the fs can be reclaimed regardless of id. Because of this, I think at a minimum we should change XFS_IOC_FREE_EOFBLOCKS to require capable(CAP_SYS_ADMIN), which somewhat implies init_user_ns based ids.

To make this really userns aware, I think we could:
- leave the fields as uid_t
- make XFS_IOC_FREE_EOFBLOCKS require nsown_capable(CAP_SYS_ADMIN)
- check kuid_has_mapping(current_user_ns()) for each inode. This would be a change in behavior when called from !init_user_ns, limiting the scope of inodes the ioctl could affect.
- change xfs_inode_match_id() to use uid_eq(VFS_I(ip)->i_uid,

    make_kuid(current_user_ns(), eofb->eof_uid))

Does that sound reasonable?

Quoted 22 lines

Quoted 22 lines

fs/xfs/xfs_icache.c +0 -0

index 96e344e..70ba410 100644
@@ -617,7 +617,7 @@ restart:
 
 /*
  * Background scanning to trim post-EOF preallocated space. This

is queued
- * based on the 'background_prealloc_discard_period' tunable (5m by default).
+ * based on the 'speculative_prealloc_lifetime' tunable (5m by default). */
STATIC void
xfs_queue_eofblocks(

fs/xfs/xfs_inode.c +0 -0

index 7f7be5f..8049976 100644
@@ -1268,8 +1268,8 @@ xfs_ialloc(
 	ip->i_d.di_onlink = 0;
 	ip->i_d.di_nlink = nlink;

Quoted 8 lines

fs/xfs/xfs_inode.c xfs_ialloc( · line 1272

Quoted 5 lines

fs/xfs/xfs_inode.c +2 -2

 	ASSERT(ip->i_d.di_nlink == nlink);
-	ip->i_d.di_uid = current_fsuid();
-	ip->i_d.di_gid = current_fsgid();
+	ip->i_d.di_uid = from_kuid(&init_user_ns, current_fsuid());
+	ip->i_d.di_gid = from_kgid(&init_user_ns, current_fsgid());

Why all new inodes be created in the init_user_ns? Shouldn't this be current_user_ns()?

current_fsuid() is the kuid_t from whatever userns current is in, which we are converting to a flat uint32 since i_d is the on disk inode. This field is then used in xfs_setup_inode() to populate VFS_I(ip)->i_uid. Most other filesystems would use inode_init_owner(), but xfs does not (I assume because it wants to handle SGID itself based on XFS_INHERIT_GID and irix_sgid_inherit).

Quoted 3 lines

Same question throughout - why do you use init_user_ns for all these UID conversions, when the whole point is to have awareness of different namespaces?

Yep, there are instances you point out below where we can just use inode->i_uid instead of converting back from the flat value, so I'll fix those up.

Quoted 100 lines

Quoted 17 lines

fs/xfs/xfs_inode.c +1 -1

 	xfs_set_projid(ip, prid);
 	memset(&(ip->i_d.di_pad[0]), 0, sizeof(ip->i_d.di_pad));
 

@@ -1308,7 +1308,7 @@ xfs_ialloc(
 	 */
 	if ((irix_sgid_inherit) &&
 	    (ip->i_d.di_mode & S_ISGID) &&
-	    (!in_group_p((gid_t)ip->i_d.di_gid))) {
+	    (!in_group_p(make_kgid(&init_user_ns,
 ip->i_d.di_gid)))) { ip->i_d.di_mode &= ~S_ISGID;
 	}
 

fs/xfs/xfs_ioctl.c +0 -0

index 5e99968..daa6127 100644

fs/xfs/xfs_ioctl.c xfs_ioctl_setattr( · line 985

Quoted 6 lines

fs/xfs/xfs_ioctl.c +1 -1

@@ -981,7 +981,7 @@ xfs_ioctl_setattr(
 	 * to the file owner ID, except in cases where the
 	 * CAP_FSETID capability is applicable.
 	 */
-	if (current_fsuid() != ip->i_d.di_uid
 && !capable(CAP_FOWNER)) {
+	if (!inode_owner_or_capable(&ip->i_vnode)) {

				    VFS_I(ip)

Quoted 8 lines

fs/xfs/xfs_ioctl.c +0 -0

 		code = XFS_ERROR(EPERM);
 		goto error_return;

fs/xfs/xfs_ioctl.c +0 -0

 	}

fs/xfs/xfs_iops.c +0 -0

index ca9ecaa..bf96cf8 100644

fs/xfs/xfs_iops.c xfs_vn_getattr( · line 424

Quoted 7 lines

fs/xfs/xfs_iops.c +2 -2

@@ -420,8 +420,8 @@ xfs_vn_getattr(
 	stat->dev = inode->i_sb->s_dev;
 	stat->mode = ip->i_d.di_mode;
 	stat->nlink = ip->i_d.di_nlink;
-	stat->uid = ip->i_d.di_uid;
-	stat->gid = ip->i_d.di_gid;
+	stat->uid = make_kuid(&init_user_ns, ip->i_d.di_uid);
+	stat->gid = make_kgid(&init_user_ns, ip->i_d.di_gid);

Why not:

	stat->uid = inode->i_uid;
	stat->gid = inode->i_gid;

Quoted 16 lines

fs/xfs/xfs_iops.c +2 -2

 	stat->ino = ip->i_ino;
 	stat->atime = inode->i_atime;
 	stat->mtime = inode->i_mtime;

@@ -488,8 +488,8 @@ xfs_setattr_nonsize(
 	int			mask = iattr->ia_valid;
 	xfs_trans_t		*tp;
 	int			error;
-	uid_t			uid = 0, iuid = 0;
-	gid_t			gid = 0, igid = 0;
+	kuid_t			uid = GLOBAL_ROOT_UID, iuid
 = GLOBAL_ROOT_UID;
+	kgid_t			gid = GLOBAL_ROOT_GID, igid

= GLOBAL_ROOT_GID; struct xfs_dquot *udqp = NULL, *gdqp =
NULL; struct xfs_dquot *olddquot1 = NULL, *olddquot2 = NULL;

fs/xfs/xfs_iops.c xfs_setattr_nonsize( · line 526

Quoted 6 lines

fs/xfs/xfs_iops.c +1 -1

@@ -522,13 +522,13 @@ xfs_setattr_nonsize(
 			uid = iattr->ia_uid;
 			qflags |= XFS_QMOPT_UQUOTA;
 		} else {
-			uid = ip->i_d.di_uid;
+			uid = make_kuid(&init_user_ns,
 ip->i_d.di_uid);

			uid = VFS_I(ip)->i_uid;

fs/xfs/xfs_iops.c xfs_setattr_nonsize(

Quoted 8 lines

fs/xfs/xfs_iops.c +1 -1

 		}
 		if ((mask & ATTR_GID) && XFS_IS_GQUOTA_ON(mp)) {
 			gid = iattr->ia_gid;
 			qflags |= XFS_QMOPT_GQUOTA;
 		}  else {
-			gid = ip->i_d.di_gid;
+			gid = make_kgid(&init_user_ns,
 ip->i_d.di_gid);

			gid = VFS_I(ip)->i_gid;

Quoted 1 line

fs/xfs/xfs_iops.c +0 -0

 		}

.....

fs/xfs/xfs_iops.c xfs_setattr_nonsize( · line 570

Quoted 11 lines

fs/xfs/xfs_iops.c +2 -2

@@ -561,8 +563,8 @@ xfs_setattr_nonsize(
 		 * while we didn't have the inode locked, inode's
 dquot(s)
 		 * would have changed also.
 		 */
-		iuid = ip->i_d.di_uid;
-		igid = ip->i_d.di_gid;
+		iuid = make_kuid(&init_user_ns, ip->i_d.di_uid);
+		igid = make_kgid(&init_user_ns, ip->i_d.di_gid);
 		gid = (mask & ATTR_GID) ? iattr->ia_gid : igid;
 		uid = (mask & ATTR_UID) ? iattr->ia_uid : iuid;

Same again - you can just use VFS_I(ip)->i_uid/VFS_I(ip)->i_gid here.

Quoted 2 lines

fs/xfs/xfs_iops.c +0 -0

@@ -1172,8 +1174,8 @@ xfs_setup_inode(
 

Quoted 10 lines

fs/xfs/xfs_iops.c xfs_setup_inode(

Quoted 8 lines

fs/xfs/xfs_iops.c +2 -2

 	inode->i_mode	= ip->i_d.di_mode;
 	set_nlink(inode, ip->i_d.di_nlink);
-	inode->i_uid	= ip->i_d.di_uid;
-	inode->i_gid	= ip->i_d.di_gid;
+	inode->i_uid	= make_kuid(&init_user_ns,
 ip->i_d.di_uid);
+	inode->i_gid	= make_kgid(&init_user_ns,
 ip->i_d.di_gid);

current name space?

I believe that is what this is doing, but I think it will be more proper to do it the same as other filesystems:

i_uid_write(inode, ip->i_d.di_uid);
i_gid_write(inode, ip->i_d.di_gid);

Quoted 18 lines

Quoted 8 lines

fs/xfs/xfs_iops.c +0 -0

 	switch (inode->i_mode & S_IFMT) {

fs/xfs/xfs_iops.c +0 -0

 	case S_IFBLK:

fs/xfs/xfs_qm.c +0 -0

index b75c9bb..94a2a8f 100644

fs/xfs/xfs_qm.c xfs_qm_write_sb_changes( · line 1655

Quoted 7 lines

fs/xfs/xfs_qm.c +2 -2

@@ -1651,8 +1651,8 @@ xfs_qm_write_sb_changes(
 int
 xfs_qm_vop_dqalloc(
 	struct xfs_inode	*ip,
-	uid_t			uid,
-	gid_t			gid,
+	__uint32_t		di_uid,
+	__uint32_t		di_gid,

xfs_dqid_t

Quoted 3 lines

And there's no need to rename the variables - that just causes unnecessary churn, and the fact it is a dquot ID is documented by the type.

Yep using the type will be clearer, thanks.

Quoted 3 lines

Cheers,

Dave.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 46 lines · Dwight Engen

Quoted 12 lines · On Thu, 20 Jun 2013 10:13:41 +1000 Dave Chinner <[email protected]> wrote:

Quoted 5 lines · Dwight Engen

Use uint32 from init_user_ns for xfs internal uid/gid representation in acl, xfs_icdinode. Conversion of kuid/gid is done at the vfs boundary, other user visible xfs specific interfaces (bulkstat, eofblocks filter) expect uint32 init_user_ns uid/gid values.

It's minimal, but I'm not sure it's complete. I'll comment on that in response to Eric's comments...

Quoted 1 line

Signed-off-by Dwight Engen <[email protected]>

....

Quoted 16 lines

fs/xfs/xfs_fs.h typedef struct xfs_error_injection { · line 354

Quoted 13 lines

fs/xfs/xfs_fs.h +2 -2

@@ -347,8 +347,8 @@ typedef struct xfs_error_injection {
 struct xfs_eofblocks {
 	__u32		eof_version;
 	__u32		eof_flags;
-	uid_t		eof_uid;
-	gid_t		eof_gid;
+	__u32		eof_uid;
+	__u32		eof_gid;
 	prid_t		eof_prid;
 	__u32		pad32;
 	__u64		eof_min_file_size;

The patch doesn't do namespace conversion of these uid/gids, but I'm not sure it should...

The ids are only advisory, if the caller doesn't specify XFS_EOF_FLAGS_?ID blocks from any inode in the fs can be reclaimed regardless of id. Because of this, I think at a minimum we should change XFS_IOC_FREE_EOFBLOCKS to require capable(CAP_SYS_ADMIN), which somewhat implies init_user_ns based ids.

To make this really userns aware, I think we could:
- leave the fields as uid_t
- make XFS_IOC_FREE_EOFBLOCKS require nsown_capable(CAP_SYS_ADMIN)
- check kuid_has_mapping(current_user_ns()) for each inode. This would be a change in behavior when called from !init_user_ns, limiting the scope of inodes the ioctl could affect.
- change xfs_inode_match_id() to use uid_eq(VFS_I(ip)->i_uid,

Quoted 4 lines

    make_kuid(current_user_ns(), eofb->eof_uid))

Does that sound reasonable?

Hi Dwight,

If I understand correctly, the proposition is to turn XFS_EOF_FREE_EOFBLOCKS into administrator only functionality and run ns conversions on the inode uid/gid and associated eofb values for the ID filtering functionality.

I am little dubious about XFS_IOC_FREE_EOFBLOCKS allowing any user to affect any other user. Your changes just seem to make it guaranteed that when called from a user namespace the wrong user will be affected.

The latter sounds reasonable to me, though I'm not so sure about the CAP_SYS_ADMIN bit. For example, I think we'd expect a regular user to be able to run an eofblocks scan against files covered under his quota.

Perhaps the right thing to do here is to restrict global (and project quota?) scans to CAP_SYS_ADMIN and uid/gid based scans to processes with the appropriate permissions (i.e., CAP_SYS_ADMIN, matching uid/gid or CAP_FOWNER). Thoughts?

Brian

Quoted 5 lines · On Thu, 20 Jun 2013 11:27:04 -0400 Brian Foster <[email protected]> wrote:

Quoted 4 lines · Dwight Engen

Quoted 1 line · On Thu, 20 Jun 2013 10:13:41 +1000 Dave Chinner <[email protected]> wrote:

On Wed, Jun 19, 2013 at 11:09:48AM -0400, Dwight Engen wrote:

Quoted 7 lines

Hi Dwight,

If I understand correctly, the proposition is to turn XFS_EOF_FREE_EOFBLOCKS into administrator only functionality and run ns conversions on the inode uid/gid and associated eofb values for the ID filtering functionality.

Hi Brian, yeah that was the proposal :) I think there are really two issues here. One is that the uid_t/gid_t may come from a userns so we should be aware of that. Currently the ids passed in are used for *filtering* so a malicious user can't do anything more than they already can by not passing ids at all, but we should fix this so only the intended files are affected. Second is that currently the ioctl allows an unprivileged user to affect another user (as Eric pointed out):

Quoted 4 lines

I am little dubious about XFS_IOC_FREE_EOFBLOCKS allowing any user to affect any other user. Your changes just seem to make it guaranteed that when called from a user namespace the wrong user will be affected.

I don't think the nsown_capability() I proposed is enough to take care of this. Do you agree that if the caller is going to affect other users, they should be CAP_SYS_ADMIN (or maybe CAP_FOWNER) in init_user_ns?

Quoted 5 lines

The latter sounds reasonable to me, though I'm not so sure about the CAP_SYS_ADMIN bit. For example, I think we'd expect a regular user to be able to run an eofblocks scan against files covered under his quota.

Quoted 4 lines

Perhaps the right thing to do here is to restrict global (and project quota?) scans to CAP_SYS_ADMIN and uid/gid based scans to processes with the appropriate permissions (i.e., CAP_SYS_ADMIN, matching uid/gid or CAP_FOWNER). Thoughts?

That sounds good to me. Maybe for a regular user the appropriate permission check (at the top of xfs_inode_free_eofblocks()) could be something like:

	if (!capable(CAP_SYS_ADMIN) &&
	    !uid_eq(VFS_I(ip)->i_uid, current_fsuid()) &&
	    !in_group_p(VFS_I(ip)->i_gid))
		return 0;

This has the drawback that the caller won't know if they supplied a uid/gid in eofblocks that won't actually get cleared, so maybe we want to validate a uid/gid in eofblocks after its copy_from_user()ed in instead? Also, I'm not sure if this is the same as "under his quota" and how it plays with project quotas.

Quoted 1 line

Brian

Quoted 12 lines · On Thu, 20 Jun 2013 11:41:33 +1000 Dave Chinner <[email protected]> wrote:

Quoted 11 lines · Eric W. Biederman

I am copying my gmail address so I have a chance of seeing replies from Dave Chiner. So far the only way I have been able to read his replies has been to read mailling lists. Which has not be conductive to having this code discussed properly. Hopefully copying my gmail address will allow us to have a reasonable and timely conversation.

Dwight Engen <[email protected]> writes:

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Use uint32 from init_user_ns for xfs internal uid/gid representation in acl, xfs_icdinode.

From my review of the code earlier that just isn't safe. It allows all kinds of things to slip through.

Such as?

Maybe saying "at the vfs boundary" is misleading, I guess I don't see how this is all that different from what you did in the other filesystems. Using ext4 as the example the conversions are done between:
struct inode <-> struct ext4_inode
struct posix_acl <-> ext4_acle_entry

which in xfs is analogous to
struct inode <-> struct xfs_icdinode
struct posix acl <-> struct xfs_acl_entry

which is where I did the conversions.

Quoted 3 lines

The kuid_t/kgid_t is actually pushed down this far - it's in the struct inode - the code currently uses the on-disk XFS uid/gid, not the struct inode's kuid_t/kgid_t. That's easily fixable.

Yep, I'll go through the code and switch to the inode where possible.

Quoted 9 lines · On Thu, 20 Jun 2013 10:13:41 +1000 Dave Chinner <[email protected]> wrote:

Quoted 5 lines · Dwight Engen

Use uint32 from init_user_ns for xfs internal uid/gid representation in acl, xfs_icdinode. Conversion of kuid/gid is done at the vfs boundary, other user visible xfs specific interfaces (bulkstat, eofblocks filter) expect uint32 init_user_ns uid/gid values.

It's minimal, but I'm not sure it's complete. I'll comment on that in response to Eric's comments...

Quoted 7 lines

Quoted 7 lines

fs/xfs/xfs_inode.c +0 -0

index 7f7be5f..8049976 100644
@@ -1268,8 +1268,8 @@ xfs_ialloc(
 	ip->i_d.di_onlink = 0;
 	ip->i_d.di_nlink = nlink;

Quoted 8 lines

fs/xfs/xfs_inode.c xfs_ialloc( · line 1272

Quoted 5 lines

fs/xfs/xfs_inode.c +2 -2

 	ASSERT(ip->i_d.di_nlink == nlink);
-	ip->i_d.di_uid = current_fsuid();
-	ip->i_d.di_gid = current_fsgid();
+	ip->i_d.di_uid = from_kuid(&init_user_ns, current_fsuid());
+	ip->i_d.di_gid = from_kgid(&init_user_ns, current_fsgid());

Why all new inodes be created in the init_user_ns? Shouldn't this be current_user_ns()?

current_fsuid() is the kuid_t from whatever userns current is in,

which we are converting to a flat uint32 since i_d is the on disk inode.

This field is then used in xfs_setup_inode() to populate VFS_I(ip)->i_uid.

Most other filesystems would use inode_init_owner(),
but xfs does not (I assume because it wants to handle SGID itself based on XFS_INHERIT_GID and irix_sgid_inherit).

Quoted 12 lines

fs/xfs/xfs_inode.c xfs_setup_inode( · line 1180

Quoted 10 lines

fs/xfs/xfs_inode.c +2 -2

@@ -1172,8 +1174,8 @@ xfs_setup_inode(
 
 	inode->i_mode	= ip->i_d.di_mode;
 	set_nlink(inode, ip->i_d.di_nlink);
-	inode->i_uid	= ip->i_d.di_uid;
-	inode->i_gid	= ip->i_d.di_gid;
+	inode->i_uid	= make_kuid(&init_user_ns,
 ip->i_d.di_uid);
+	inode->i_gid	= make_kgid(&init_user_ns,
 ip->i_d.di_gid);

current name space?

I believe that is what this is doing, but I think it will be more proper to do it the same as other filesystems:

i_uid_write(inode, ip->i_d.di_uid);
i_gid_write(inode, ip->i_d.di_gid);

Quoted 8 lines · Dwight Engen

Quoted 5 lines · On Thu, 20 Jun 2013 11:27:04 -0400 Brian Foster <[email protected]> wrote:

Quoted 4 lines · Dwight Engen

Quoted 1 line · On Thu, 20 Jun 2013 10:13:41 +1000 Dave Chinner <[email protected]> wrote:

On Wed, Jun 19, 2013 at 11:09:48AM -0400, Dwight Engen wrote:

...

Quoted 22 lines

Quoted 7 lines

Hi Dwight,

If I understand correctly, the proposition is to turn XFS_EOF_FREE_EOFBLOCKS into administrator only functionality and run ns conversions on the inode uid/gid and associated eofb values for the ID filtering functionality.

Hi Brian, yeah that was the proposal :) I think there are really two issues here. One is that the uid_t/gid_t may come from a userns so we should be aware of that. Currently the ids passed in are used for *filtering* so a malicious user can't do anything more than they already can by not passing ids at all, but we should fix this so only the intended files are affected. Second is that currently the ioctl allows an unprivileged user to affect another user (as Eric pointed out):

Quoted 4 lines

I am little dubious about XFS_IOC_FREE_EOFBLOCKS allowing any user to affect any other user. Your changes just seem to make it guaranteed that when called from a user namespace the wrong user will be affected.

Quoted 5 lines

I don't think the nsown_capability() I proposed is enough to take care of this. Do you agree that if the caller is going to affect other users, they should be CAP_SYS_ADMIN (or maybe CAP_FOWNER) in init_user_ns?

Yeah, that's what I was getting at below by restricting "global" scans to admin privilege.

Quoted 10 lines

Quoted 9 lines

The latter sounds reasonable to me, though I'm not so sure about the CAP_SYS_ADMIN bit. For example, I think we'd expect a regular user to be able to run an eofblocks scan against files covered under his quota.

Perhaps the right thing to do here is to restrict global (and project quota?) scans to CAP_SYS_ADMIN and uid/gid based scans to processes with the appropriate permissions (i.e., CAP_SYS_ADMIN, matching uid/gid or CAP_FOWNER). Thoughts?

Quoted 4 lines

That sounds good to me. Maybe for a regular user the appropriate permission check (at the top of xfs_inode_free_eofblocks()) could be something like:

I think the various capability/permission checks should be in the ioctl code.

This would still allow use cases such as the pending code I have that invokes an eofblocks scan on write() failure due to EDQUOT/ENOSPC in the case of project or user/group quotas.

I suspect adding the namespace
conversion stuff wouldn't break the typical user/group quota case, but

we'd still require the ability to run a project quota scan from a particular user context. I think the combined check you have above would break that.

Quoted 8 lines · Dwight Engen

Quoted 5 lines · On Thu, 20 Jun 2013 11:27:04 -0400 Brian Foster <[email protected]> wrote:

Quoted 4 lines · Dwight Engen

Quoted 1 line · On Thu, 20 Jun 2013 10:13:41 +1000 Dave Chinner <[email protected]> wrote:

On Wed, Jun 19, 2013 at 11:09:48AM -0400, Dwight Engen wrote:

...

Quoted 22 lines

Quoted 7 lines

Hi Dwight,

If I understand correctly, the proposition is to turn XFS_EOF_FREE_EOFBLOCKS into administrator only functionality and run ns conversions on the inode uid/gid and associated eofb values for the ID filtering functionality.

Hi Brian, yeah that was the proposal :) I think there are really two issues here. One is that the uid_t/gid_t may come from a userns so we should be aware of that. Currently the ids passed in are used for *filtering* so a malicious user can't do anything more than they already can by not passing ids at all, but we should fix this so only the intended files are affected. Second is that currently the ioctl allows an unprivileged user to affect another user (as Eric pointed out):

Quoted 4 lines

I am little dubious about XFS_IOC_FREE_EOFBLOCKS allowing any user to affect any other user. Your changes just seem to make it guaranteed that when called from a user namespace the wrong user will be affected.

Quoted 5 lines

I don't think the nsown_capability() I proposed is enough to take care of this. Do you agree that if the caller is going to affect other users, they should be CAP_SYS_ADMIN (or maybe CAP_FOWNER) in init_user_ns?

Yeah, that's what I was getting at below by restricting "global" scans to admin privilege.

Quoted 5 lines

Quoted 5 lines

The latter sounds reasonable to me, though I'm not so sure about the CAP_SYS_ADMIN bit. For example, I think we'd expect a regular user to be able to run an eofblocks scan against files covered under his quota.

Quoted 9 lines

Quoted 4 lines

Perhaps the right thing to do here is to restrict global (and project quota?) scans to CAP_SYS_ADMIN and uid/gid based scans to processes with the appropriate permissions (i.e., CAP_SYS_ADMIN, matching uid/gid or CAP_FOWNER). Thoughts?

That sounds good to me. Maybe for a regular user the appropriate permission check (at the top of xfs_inode_free_eofblocks()) could be something like:

I think the various capability/permission checks should be in the ioctl code. xfs_icache_free_eofblocks() and below are internal interfaces where these checks are probably not relevant. I actually have code lying around that creates an internal structure for that code, similar but separate from the xfs_eofblocks structure.

Quoted 5 lines

	if (!capable(CAP_SYS_ADMIN) &&
	    !uid_eq(VFS_I(ip)->i_uid, current_fsuid()) &&
	    !in_group_p(VFS_I(ip)->i_gid))
		return 0;

This is a little confusing (and pardon me, I'm a bit new to the namespace work).

What might be a bit more clear is to do the capability checks against the EOFBLOCKS command flags in xfs_file_ioctl() and return an appropriate error if permission is not granted for the requested type of scan (i.e., a regular user doing a global or non-id matching scan). Then restrict the changes in xfs_icache_free_eofblocks() to just dealing with the namespace conversions.

This would still allow use cases such as the pending code I have that invokes an eofblocks scan on write() failure due to EDQUOT/ENOSPC in the case of project or user/group quotas. I suspect adding the namespace conversion stuff wouldn't break the typical user/group quota case, but we'd still require the ability to run a project quota scan from a particular user context. I think the combined check you have above would break that.

I have a question about the project quota. Is it intended that any user can set an project quota on any file? Unless I am misreading xfs_ioctl_setattr that is what it allows.

My narrow focus concern on this is that if the user is in a user namespace these ids need to be mapped before we look at them or else we will do the wrong thing.

Quoted 12 lines · Dwight Engen

Quoted 9 lines · On Thu, 20 Jun 2013 10:13:41 +1000 Dave Chinner <[email protected]> wrote:

Quoted 5 lines · Dwight Engen

Use uint32 from init_user_ns for xfs internal uid/gid representation in acl, xfs_icdinode. Conversion of kuid/gid is done at the vfs boundary, other user visible xfs specific interfaces (bulkstat, eofblocks filter) expect uint32 init_user_ns uid/gid values.

It's minimal, but I'm not sure it's complete. I'll comment on that in response to Eric's comments...

...

Quoted 18 lines

Quoted 17 lines

Quoted 5 lines

fs/xfs/xfs_inode.c +0 -0

index 7f7be5f..8049976 100644

fs/xfs/xfs_inode.c xfs_ialloc( · line 1274

Quoted 9 lines

fs/xfs/xfs_inode.c +2 -2

@@ -1268,8 +1268,8 @@ xfs_ialloc(
 	ip->i_d.di_onlink = 0;
 	ip->i_d.di_nlink = nlink;
 	ASSERT(ip->i_d.di_nlink == nlink);
-	ip->i_d.di_uid = current_fsuid();
-	ip->i_d.di_gid = current_fsgid();
+	ip->i_d.di_uid = from_kuid(&init_user_ns,
 current_fsuid());
+	ip->i_d.di_gid = from_kgid(&init_user_ns,
 current_fsgid());

Why all new inodes be created in the init_user_ns? Shouldn't this be current_user_ns()?

Quoted 1 line

current_fsuid() is the kuid_t from whatever userns current is in,

Yes.

Quoted 2 lines

which we are converting to a flat uint32 since i_d is the on disk inode.

So, init_user_ns is actually the target namespace of the conversion, not the namespace we are converting *from*?

<sigh>

I *knew* this was going to happen when I first saw this code:

http://www.spinics.net/lists/netdev/msg209217.html

"For those of us that have to look at it once every few months, following the same conventions as all the other code in the kernel (i.e. kqid_to_id()) tells me everything I need to know without having to go through the process of looking up the unusual from_kqid() function and then from_kuid() to find out what it is actually doing...."

Here I am, several months later and trying to work out what the damn conversion from_kgid() and make_kuid() are supposed to be doing and trying to work out if it is correct or not...

Quoted 2 lines

This field is then used in xfs_setup_inode() to populate VFS_I(ip)->i_uid.

But that then uses make_kuid(&init_user_ns, ip->i_d.di_uid) which according to the documentation creates a kuid in the init_user_ns, not in the current user namespace.

So if we then do uid_eq(current_fsuid(), VFS_I(ip)->i_uid) on the newly created and initialised inode they are different, yes? If they are different, then this code is not correct....

Quoted 3 lines

Most other filesystems would use inode_init_owner(), but xfs does not (I assume because it wants to handle SGID itself based on XFS_INHERIT_GID and irix_sgid_inherit).

No, XFS doesn't use inode_init_owner() because we are initialising the on disk XFS inode here, not the VFS struct inode...

Quoted 13 lines

Quoted 12 lines

fs/xfs/xfs_inode.c xfs_setup_inode( · line 1180

Quoted 10 lines

fs/xfs/xfs_inode.c +2 -2

@@ -1172,8 +1174,8 @@ xfs_setup_inode(
 
 	inode->i_mode	= ip->i_d.di_mode;
 	set_nlink(inode, ip->i_d.di_nlink);
-	inode->i_uid	= ip->i_d.di_uid;
-	inode->i_gid	= ip->i_d.di_gid;
+	inode->i_uid	= make_kuid(&init_user_ns,
 ip->i_d.di_uid);
+	inode->i_gid	= make_kgid(&init_user_ns,
 ip->i_d.di_gid);

current name space?

Quoted 5 lines

I believe that is what this is doing, but I think it will be more proper to do it the same as other filesystems:

i_uid_write(inode, ip->i_d.di_uid);
i_gid_write(inode, ip->i_d.di_gid);

Sure, but that's still creating the uids/gids in the init_user_ns, so it doesn't solve my confusion about *why* this is being done. There's no documentation as to how this stuff is supposed to work, so I can't find out for myself. I'm not one for cargo-cult copy-n-paste development - I like to understand why something is done before copying it...

So, to prevent me from wondering what it is doing in another 6 months time, can you add a set of helper functions that are named:

xfs_kuid_to_disk()
xfs_kuid_from_disk()
xfs_kgid_to_disk()
xfs_kgid_from_disk()

and document why we are using the namespaces that are being used, and then use them where we convert to/from the different inode structures?

FWIW, what happens when ip->i_d.di_gid doesn't have a mapping in the current namespace, and make_kuid/make_kgid return INVALID_UID/INVALID_GID? Is this is going to happen, and if it does what do we need to do about it? That will need to be added to the comments, too.

At least if we get this done, XFS people will be able to tell at a glance that the XFs code is doing the right thing w.r.t namespace conversion...

Cheers,

Dave.

Quoted 34 lines · On Fri, 21 Jun 2013 08:03:11 +1000 Dave Chinner <[email protected]> wrote:

Quoted 12 lines · Dwight Engen

Quoted 9 lines · On Thu, 20 Jun 2013 10:13:41 +1000 Dave Chinner <[email protected]> wrote:

Quoted 5 lines · Dwight Engen

Use uint32 from init_user_ns for xfs internal uid/gid representation in acl, xfs_icdinode. Conversion of kuid/gid is done at the vfs boundary, other user visible xfs specific interfaces (bulkstat, eofblocks filter) expect uint32 init_user_ns uid/gid values.

It's minimal, but I'm not sure it's complete. I'll comment on that in response to Eric's comments...

...

Quoted 19 lines

Quoted 9 lines

Quoted 9 lines

fs/xfs/xfs_inode.c +0 -1

index 7f7be5f..8049976 100644
@@ -1268,8 +1268,8 @@ xfs_ialloc(
 	ip->i_d.di_onlink = 0;
 	ip->i_d.di_nlink = nlink;
 	ASSERT(ip->i_d.di_nlink == nlink);
-	ip->i_d.di_uid = current_fsuid();

Quoted 8 lines

fs/xfs/xfs_inode.c xfs_ialloc( · line 1274

Quoted 5 lines

fs/xfs/xfs_inode.c +2 -1

-	ip->i_d.di_gid = current_fsgid();
+	ip->i_d.di_uid = from_kuid(&init_user_ns,
 current_fsuid());
+	ip->i_d.di_gid = from_kgid(&init_user_ns,
 current_fsgid());

Why all new inodes be created in the init_user_ns? Shouldn't this be current_user_ns()?

current_fsuid() is the kuid_t from whatever userns current is in,

Quoted 4 lines

Yes.

Quoted 2 lines

which we are converting to a flat uint32 since i_d is the on disk inode.

I was incorrect here. current_fsuid() is the kuid_t from the perspective of init_user_ns, so the value won't be different, just the type.

Quoted 20 lines

So, init_user_ns is actually the target namespace of the conversion, not the namespace we are converting *from*?

<sigh>

I *knew* this was going to happen when I first saw this code:

http://www.spinics.net/lists/netdev/msg209217.html

"For those of us that have to look at it once every few months, following the same conventions as all the other code in the kernel (i.e. kqid_to_id()) tells me everything I need to know without having to go through the process of looking up the unusual from_kqid() function and then from_kuid() to find out what it is actually doing...."

Here I am, several months later and trying to work out what the damn conversion from_kgid() and make_kuid() are supposed to be doing and trying to work out if it is correct or not...

Quoted 6 lines

Quoted 2 lines

This field is then used in xfs_setup_inode() to populate VFS_I(ip)->i_uid.

But that then uses make_kuid(&init_user_ns, ip->i_d.di_uid) which according to the documentation creates a kuid in the init_user_ns, not in the current user namespace.

Right, inside the kernel uids are all interpreted within init_user_ns context and are only converted to a different value if the caller is in some other userns at the kernel/user boundary (ie. cp_compat_stat()). Maybe it is more clear to say that init_user_ns is the "flat 32bit uid space" and thus has a mapping for every uid. So the value in init_user_ns will actually be equal to the di_uid value.

Quoted 3 lines

So if we then do uid_eq(current_fsuid(), VFS_I(ip)->i_uid) on the newly created and initialised inode they are different, yes? If they are different, then this code is not correct....

No they should equal because xfs_setup_inode() maps it back, see below.

Quoted 4 lines

Quoted 3 lines

Most other filesystems would use inode_init_owner(), but xfs does not (I assume because it wants to handle SGID itself based on XFS_INHERIT_GID and irix_sgid_inherit).

Quoted 2 lines

No, XFS doesn't use inode_init_owner() because we are initialising the on disk XFS inode here, not the VFS struct inode...

Right, but I was referring to xfs_setup_inode() which is setting up the VFS inode. It is called in both the from disk and new inode paths, and sets the VFS inode uid based on the value in the disk inode.

I was
trying to show that in the new inode path the VFS inode uid is being initialized the same as on other filesystems (ie from current_fsuid()), but with an extra step just because of "conversion" to and back from init_user_ns. The call sequence in xfs:

xfs_ialloc():

    current_fsuid() -> from_kuid() -> di_uid

xfs_setup_inode():

    di_uid -> make_kuid() -> inode.i_uid == current_fsuid()

Sorry, I think my first explanation wasn't clear, hopefully this is.

Quoted 11 lines

So, to prevent me from wondering what it is doing in another 6 months time, can you add a set of helper functions that are named:

xfs_kuid_to_disk()
xfs_kuid_from_disk()
xfs_kgid_to_disk()
xfs_kgid_from_disk()

and document why we are using the namespaces that are being used, and then use them where we convert to/from the different inode structures?

Sure I can take a shot at that, and I'm guessing you would prefer to use

inode->i_uid = xfs_kuid_from_disk(ip->i_d.di_uid);

over

i_uid_write(inode, ip->i_d.di_uid);

Quoted 5 lines

FWIW, what happens when ip->i_d.di_gid doesn't have a mapping in the current namespace, and make_kuid/make_kgid return INVALID_UID/INVALID_GID? Is this is going to happen, and if it does what do we need to do about it? That will need to be added to the comments, too.

I don't think it will happen here because init_user_ns has a mapping for all values. Where it can happen is when there is a conversion to some subset userns, that doesn't have a mapping for the value.

Hi Dave, all. Here is a v2 patch that I believe addresses the previous comments, but I expect there to be more :) I think there are a few more issues to sort out before this is ready, and I want to add some tests to xfstests also.

I added permission checks for eofblocks in the ioctl code, but I don't think they are enough. Just because an unprivileged caller is in a group doesn't mean he can write to a file of that group, and I don't know how we can check for that till we get the inode in hand. Brian, if you or anyone else could comment on how this should work for the regular user and write() ENOSPC cases that'd be great.

The xfs code now uses inode->i_uid where possible instead of di_uid. The remaining uses of di_uid are where the inode is being setup, conversion to/from disk endianess, in dealing with quotas, and bulkstat.

We do need to decide on the di_uid that comes back from bulkstat. Right now it is returning on disk (== init_user_ns) uids. It looks to me like xfsrestore is using the normal vfs routines (chown, fchown, lchown) when restoring so that won't line up if the xfsrestore is run in !init_user_ns. We could possibly convert to userns values before returning them from the kernel, but I doubt that will work well with the xfs quotas. Should we just require that callers of bulkstat be in init_user_ns? Thoughts?

--

Use uint32 from init_user_ns for xfs internal uid/gid representation in acl, xfs_icdinode, xfs_dqid_t. Conversion of kuid/gid is done for these structures and for the eofblocks filter. Other user visible xfs specific interfaces (bulkstat) expect uint32 init_user_ns uid/gid values.

Signed-off-by Dwight Engen <[email protected]>

---
 fs/xfs/xfs_acl.c      | 20 ++++++++++++++++----
 fs/xfs/xfs_fs.h       |  2 +-
 fs/xfs/xfs_icache.c   |  6 +++---
 fs/xfs/xfs_inode.c    |  6 +++---
 fs/xfs/xfs_ioctl.c    | 37 ++++++++++++++++++++++++++++++++++---
 fs/xfs/xfs_iops.c     | 38 ++++++++++++++++++++------------------
 fs/xfs/xfs_linux.h    | 35 +++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_qm.c       | 10 +++++-----
 fs/xfs/xfs_quota.h    |  9 +++++----
 fs/xfs/xfs_symlink.c  |  4 +++-
 fs/xfs/xfs_vnodeops.c |  4 +++-
 init/Kconfig          | 15 +--------------
 12 files changed, 129 insertions(+), 57 deletions(-)

fs/xfs/xfs_fs.h +0 -0

 	__u32		eof_version;
 	__u32		eof_flags;
 	uid_t		eof_uid;

fs/xfs/xfs_icache.c +1 -1

index 96e344e..2c35b13 100644
@@ -617,7 +617,7 @@ restart:
 
 /*
  * Background scanning to trim post-EOF preallocated space. This is queued
- * based on the 'background_prealloc_discard_period' tunable (5m by default).
+ * based on the 'speculative_prealloc_lifetime' tunable (5m by default).
  */
 STATIC void
 xfs_queue_eofblocks(

fs/xfs/xfs_icache.c +0 -0

 		return 0;
 
 	if (eofb->eof_flags & XFS_EOF_FLAGS_PRID &&

fs/xfs/xfs_ioctl.c +0 -0

index 5e99968..d6e64d9 100644

fs/xfs/xfs_ioctl.c +2 -1

@@ -1610,7 +1610,8 @@ xfs_file_ioctl(
 		return -error;
 
 	case XFS_IOC_FREE_EOFBLOCKS: {
-		struct xfs_eofblocks eofb;
+		struct xfs_ueofblocks eofb;
+		struct xfs_eofblocks keofb;
 
 		if (copy_from_user(&eofb, arg, sizeof(eofb)))
 			return -XFS_ERROR(EFAULT);

fs/xfs/xfs_ioctl.c +14 -0

+		if (eofb.eof_flags & XFS_EOF_FLAGS_UID) {
+			keofb.eof_uid = make_kuid(current_user_ns(), eofb.eof_uid);
+			if (!uid_valid(keofb.eof_uid))
+				return -XFS_ERROR(EINVAL);
+		}
+
+		if (eofb.eof_flags & XFS_EOF_FLAGS_GID) {
+			keofb.eof_gid = make_kgid(current_user_ns(), eofb.eof_gid);
+			if (!gid_valid(keofb.eof_gid))
+				return -XFS_ERROR(EINVAL);
+		}
+
+		if (!capable(CAP_SYS_ADMIN)) {
+			if (!(eofb.eof_flags & (XFS_EOF_FLAGS_UID | XFS_EOF_FLAGS_GID)))

fs/xfs/xfs_ioctl.c +2 -0

+
+		error = xfs_icache_free_eofblocks(mp, &keofb);
 		return -error;
 	}
 

fs/xfs/xfs_linux.h +0 -0

index 800f896..80326da 100644
@@ -159,6 +159,41 @@
 #define MAX(a,b)	(max(a,b))
 #define howmany(x, y)	(((x)+((y)-1))/(y))
 

/*
* Various platform dependent calls that don't fit anywhere else */

fs/xfs/xfs_qm.c +5 -5

index b75c9bb..57e2c18 100644
@@ -1651,8 +1651,8 @@ xfs_qm_write_sb_changes(
 int
 xfs_qm_vop_dqalloc(
 	struct xfs_inode	*ip,
-	uid_t			uid,
-	gid_t			gid,
+	xfs_dqid_t		uid,
+	xfs_dqid_t		gid,
 	prid_t			prid,
 	uint			flags,
 	struct xfs_dquot	**O_udqpp,

@@ -1697,7 +1697,7 @@ xfs_qm_vop_dqalloc(
 			 * holding ilock.
 			 */
 			xfs_iunlock(ip, lockflags);
-			if ((error = xfs_qm_dqget(mp, NULL, (xfs_dqid_t) uid,
+			if ((error = xfs_qm_dqget(mp, NULL, uid,
 						 XFS_DQ_USER,
 						 XFS_QMOPT_DQALLOC |
 						 XFS_QMOPT_DOWARN,

@@ -1723,7 +1723,7 @@ xfs_qm_vop_dqalloc(
 	if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
 		if (ip->i_d.di_gid != gid) {
 			xfs_iunlock(ip, lockflags);
-			if ((error = xfs_qm_dqget(mp, NULL, (xfs_dqid_t)gid,
+			if ((error = xfs_qm_dqget(mp, NULL, gid,
 						 XFS_DQ_GROUP,
 						 XFS_QMOPT_DQALLOC |
 						 XFS_QMOPT_DOWARN,

@@ -1842,7 +1842,7 @@ xfs_qm_vop_chown_reserve(
 			XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS;
 
 	if (XFS_IS_UQUOTA_ON(mp) && udqp &&
-	    ip->i_d.di_uid != (uid_t)be32_to_cpu(udqp->q_core.d_id)) {
+	    ip->i_d.di_uid != be32_to_cpu(udqp->q_core.d_id)) {
 		delblksudq = udqp;
 		/*
 		 * If there are delayed allocation blocks, then we have to

fs/xfs/xfs_quota.h +5 -4

index c38068f..5f0bfe8 100644
@@ -320,8 +320,8 @@ extern int xfs_trans_reserve_quota_bydquots(struct xfs_trans *,
 		struct xfs_mount *, struct xfs_dquot *,
 		struct xfs_dquot *, long, long, uint);
 
-extern int xfs_qm_vop_dqalloc(struct xfs_inode *, uid_t, gid_t, prid_t, uint,
-		struct xfs_dquot **, struct xfs_dquot **);
+extern int xfs_qm_vop_dqalloc(struct xfs_inode *, xfs_dqid_t, xfs_dqid_t,
+		prid_t, uint, struct xfs_dquot **, struct xfs_dquot **);
 extern void xfs_qm_vop_create_dqattach(struct xfs_trans *, struct xfs_inode *,
 		struct xfs_dquot *, struct xfs_dquot *);
 extern int xfs_qm_vop_rename_dqattach(struct xfs_inode **);

@@ -341,8 +341,9 @@ extern void xfs_qm_unmount_quotas(struct xfs_mount *);
 
 #else
 static inline int
-xfs_qm_vop_dqalloc(struct xfs_inode *ip, uid_t uid, gid_t gid, prid_t prid,
-		uint flags, struct xfs_dquot **udqp, struct xfs_dquot **gdqp)
+xfs_qm_vop_dqalloc(struct xfs_inode *ip, xfs_dqid_t uid, xfs_dqid_t gid,
+		prid_t prid, uint flags, struct xfs_dquot **udqp,
+		struct xfs_dquot **gdqp)
 {
 	*udqp = NULL;
 	*gdqp = NULL;

fs/xfs/xfs_symlink.c +3 -1

index 195a403..c50306e 100644
@@ -384,7 +384,9 @@ xfs_symlink(
 	/*
 	 * Make sure that we have allocated dquot(s) on disk.
 	 */
-	error = xfs_qm_vop_dqalloc(dp, current_fsuid(), current_fsgid(), prid,
+	error = xfs_qm_vop_dqalloc(dp,
+			xfs_kuid_to_disk(current_fsuid()),
+			xfs_kgid_to_disk(current_fsgid()), prid,
 			XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT, &udqp, &gdqp);
 	if (error)
 		goto std_return;

fs/xfs/xfs_vnodeops.c +3 -1

index 0176bb2..94f4f9f6 100644
@@ -515,7 +515,9 @@ xfs_create(
 	/*
 	 * Make sure that we have allocated dquot(s) on disk.
 	 */
-	error = xfs_qm_vop_dqalloc(dp, current_fsuid(), current_fsgid(), prid,
+	error = xfs_qm_vop_dqalloc(dp,
+			xfs_kuid_to_disk(current_fsuid()),
+			xfs_kgid_to_disk(current_fsgid()), prid,
 			XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT, &udqp, &gdqp);
 	if (error)
 		return error;

init/Kconfig +0 -10

index 9d3a788..8083ffd 100644
@@ -1065,7 +1065,6 @@ config IPC_NS
 
 config USER_NS
 	bool "User namespace"
-	depends on UIDGID_CONVERTED
 	select UIDGID_STRICT_TYPE_CHECKS
 
 	default n

@@ -1099,21 +1098,9 @@ config NET_NS
 
 endif # NAMESPACES
 
-config UIDGID_CONVERTED
-	# True if all of the selected software conmponents are known
-	# to have uid_t and gid_t converted to kuid_t and kgid_t
-	# where appropriate and are otherwise safe to use with
-	# the user namespace.
-	bool
-	default y
-
-	# Filesystems

Quoted 5 lines · Dwight Engen

Hi Dave, all. Here is a v2 patch that I believe addresses the previous comments, but I expect there to be more :) I think there are a few more issues to sort out before this is ready, and I want to add some tests to xfstests also.

Quoted 8 lines

I added permission checks for eofblocks in the ioctl code, but I don't think they are enough. Just because an unprivileged caller is in a group doesn't mean he can write to a file of that group, and I don't know how we can check for that till we get the inode in hand. Brian, if you or anyone else could comment on how this should work for the regular user and write() ENOSPC cases that'd be great.

Hi Dwight,

Fair point with regard to the group. On one hand, we aren't exactly writing to the file and from the perspective of the fs, we'd like the ability for a user covered under a group quota to have the ability to manage inodes covered under said quota. On the other hand, from a userspace perspective it could imply a strange incarnation/interpretation(/abuse) of file permissions. I'll have to think about this some more.

Before getting too far into the code, could we break this down into smaller, independent patches? This is starting to include logically independent changes, and on first glance, it appears even the eofblocks stuff could be broken down into multiple patches (i.e., the introduction of a new structure should be its own patch, etc.). But more on that to follow...

Quoted 39 lines

The xfs code now uses inode->i_uid where possible instead of di_uid. The remaining uses of di_uid are where the inode is being setup, conversion to/from disk endianess, in dealing with quotas, and bulkstat.

We do need to decide on the di_uid that comes back from bulkstat. Right now it is returning on disk (== init_user_ns) uids. It looks to me like xfsrestore is using the normal vfs routines (chown, fchown, lchown) when restoring so that won't line up if the xfsrestore is run in !init_user_ns. We could possibly convert to userns values before returning them from the kernel, but I doubt that will work well with the xfs quotas. Should we just require that callers of bulkstat be in init_user_ns? Thoughts?

--

Use uint32 from init_user_ns for xfs internal uid/gid representation in acl, xfs_icdinode, xfs_dqid_t. Conversion of kuid/gid is done for these structures and for the eofblocks filter. Other user visible xfs specific interfaces (bulkstat) expect uint32 init_user_ns uid/gid values.

Signed-off-by Dwight Engen <[email protected]>

---
 fs/xfs/xfs_acl.c      | 20 ++++++++++++++++----
 fs/xfs/xfs_fs.h       |  2 +-
 fs/xfs/xfs_icache.c   |  6 +++---
 fs/xfs/xfs_inode.c    |  6 +++---
 fs/xfs/xfs_ioctl.c    | 37 ++++++++++++++++++++++++++++++++++---
 fs/xfs/xfs_iops.c     | 38 ++++++++++++++++++++------------------
 fs/xfs/xfs_linux.h    | 35 +++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_qm.c       | 10 +++++-----
 fs/xfs/xfs_quota.h    |  9 +++++----
 fs/xfs/xfs_symlink.c  |  4 +++-
 fs/xfs/xfs_vnodeops.c |  4 +++-
 init/Kconfig          | 15 +--------------
 12 files changed, 129 insertions(+), 57 deletions(-)

...

fs/xfs/xfs_fs.h typedef struct xfs_error_injection { · line 347

Quoted 10 lines

fs/xfs/xfs_fs.h +1 -1

index d046955..6bc3da4 100644
@@ -344,7 +344,7 @@ typedef struct xfs_error_injection {
  * Speculative preallocation trimming.
  */
 #define XFS_EOFBLOCKS_VERSION		1
-struct xfs_eofblocks {
+struct xfs_ueofblocks {

We probably don't want to go and change the name of the structure exported to userspace. This was intentionally named with the understanding that it's user facing. If anything, I'd say leave this and use a different name for the internal version.

FWIW, I already have some patches that create an internal version of xfs_eofblocks for a separate purpose (adding a new internal only field). As is, it might not completely meet the use case here, but I'm wondering if we should combine some efforts here...

Quoted 26 lines

fs/xfs/xfs_fs.h +0 -0

 	__u32		eof_version;
 	__u32		eof_flags;
 	uid_t		eof_uid;

fs/xfs/xfs_icache.c +0 -0

index 96e344e..2c35b13 100644
@@ -617,7 +617,7 @@ restart:
 
 /*
  * Background scanning to trim post-EOF preallocated space. This

is queued
- * based on the 'background_prealloc_discard_period' tunable (5m by default).
+ * based on the 'speculative_prealloc_lifetime' tunable (5m by default). */
STATIC void
xfs_queue_eofblocks(

fs/xfs/xfs_icache.c +1 -1

@@ -1202,11 +1202,11 @@ xfs_inode_match_id(
 	struct xfs_eofblocks	*eofb)
 {
 	if (eofb->eof_flags & XFS_EOF_FLAGS_UID &&
-	    ip->i_d.di_uid != eofb->eof_uid)
+	    !uid_eq(VFS_I(ip)->i_uid, eofb->eof_uid))
 		return 0;
 

fs/xfs/xfs_icache.c xfs_inode_match_id( · line 1209

Quoted 3 lines

fs/xfs/xfs_icache.c +1 -1

 	if (eofb->eof_flags & XFS_EOF_FLAGS_GID &&
-	    ip->i_d.di_gid != eofb->eof_gid)
+	    !gid_eq(VFS_I(ip)->i_gid, eofb->eof_gid))

More of a question... since we're originally comparing against the on-disk values, is the separate internal structure strictly necessary for making eofblocks userns aware?

Quoted 3 lines

fs/xfs/xfs_icache.c +0 -0

 		return 0;
 
 	if (eofb->eof_flags & XFS_EOF_FLAGS_PRID &&

...

Quoted 4 lines

fs/xfs/xfs_ioctl.c +0 -0

index 5e99968..d6e64d9 100644

...

Quoted 14 lines

fs/xfs/xfs_ioctl.c +2 -1

@@ -1610,7 +1610,8 @@ xfs_file_ioctl(
 		return -error;
 
 	case XFS_IOC_FREE_EOFBLOCKS: {
-		struct xfs_eofblocks eofb;
+		struct xfs_ueofblocks eofb;
+		struct xfs_eofblocks keofb;
 
 		if (copy_from_user(&eofb, arg, sizeof(eofb)))
 			return -XFS_ERROR(EFAULT);

@@ -1625,7 +1626,37 @@ xfs_file_ioctl(
 		    memchr_inv(eofb.pad64, 0, sizeof(eofb.pad64)))
 			return -XFS_ERROR(EINVAL);
 

fs/xfs/xfs_ioctl.c xfs_file_ioctl( · line 1633

Quoted 6 lines

fs/xfs/xfs_ioctl.c +5 -1

-		error = xfs_icache_free_eofblocks(mp, &eofb);
+		keofb.eof_version = eofb.eof_version;
+		keofb.eof_flags = eofb.eof_flags;
+		keofb.eof_prid = eofb.eof_prid;
+		keofb.eof_min_file_size = eofb.eof_min_file_size;
+

A conversion helper would be nice here. I'd probably just convert all of the fields and then reduce the following EINVAL bits to pure permission/sanity checks.

Quoted 26 lines

fs/xfs/xfs_ioctl.c +18 -0

+		if (eofb.eof_flags & XFS_EOF_FLAGS_UID) {
+			keofb.eof_uid =
 make_kuid(current_user_ns(), eofb.eof_uid);
+			if (!uid_valid(keofb.eof_uid))
+				return -XFS_ERROR(EINVAL);
+		}
+
+		if (eofb.eof_flags & XFS_EOF_FLAGS_GID) {
+			keofb.eof_gid =
 make_kgid(current_user_ns(), eofb.eof_gid);
+			if (!gid_valid(keofb.eof_gid))
+				return -XFS_ERROR(EINVAL);
+		}
+
+		if (!capable(CAP_SYS_ADMIN)) {
+			if (!(eofb.eof_flags & (XFS_EOF_FLAGS_UID
 | XFS_EOF_FLAGS_GID)))
+				return -XFS_ERROR(EPERM);
+
+			if ((eofb.eof_flags & XFS_EOF_FLAGS_UID) &&
+			    !uid_eq(current_fsuid(),

keofb.eof_uid))

fs/xfs/xfs_ioctl.c +4 -0

+				return -XFS_ERROR(EPERM);
+
+			if ((eofb.eof_flags & XFS_EOF_FLAGS_GID) &&
+			    !in_group_p(keofb.eof_gid))

fs/xfs/xfs_ioctl.c

Quoted 2 lines

fs/xfs/xfs_ioctl.c +2 -0

+				return -XFS_ERROR(EPERM);
+		}

A comment above this hunk to describe what we decide to enforce here and why will probably be helpful. ;) Otherwise, the checks seem reasonable to me, notwithstanding the open question you called out in the commit log description. FWIW, this particular EPERM check should probably stand on its own as an independent patch.

Quoted 5 lines

fs/xfs/xfs_ioctl.c +2 -0

+
+		error = xfs_icache_free_eofblocks(mp, &keofb);
 		return -error;
 	}

...

Quoted 8 lines

fs/xfs/xfs_linux.h +0 -0

index 800f896..80326da 100644
@@ -159,6 +159,41 @@
 #define MAX(a,b)	(max(a,b))
 #define howmany(x, y)	(((x)+((y)-1))/(y))
 

...

fs/xfs/xfs_linux.h line 196

Quoted 10 lines

fs/xfs/xfs_linux.h +10 -0

+
+struct xfs_eofblocks {
+	__u32		eof_version;
+	__u32		eof_flags;
+	kuid_t		eof_uid;
+	kgid_t		eof_gid;
+	prid_t		eof_prid;
+	__u64		eof_min_file_size;
+};
+

This should probably go into xfs_icache.h along with the aforementioned conversion helper.

As I mentioned previously, I have some code around that creates an internal version of the eofblocks structure. The main differences are the name (xfs_eofblocks_internal) and I did the conversion down in xfs_icache.c since I wasn't changing anything that affected the ioctl().

I can throw it up on the list for reference or if it's of any use as a base for this work...

Brian

Quoted 142 lines

/*
* Various platform dependent calls that don't fit anywhere else */

fs/xfs/xfs_qm.c +3 -3

index b75c9bb..57e2c18 100644
@@ -1651,8 +1651,8 @@ xfs_qm_write_sb_changes(
 int
 xfs_qm_vop_dqalloc(
 	struct xfs_inode	*ip,
-	uid_t			uid,
-	gid_t			gid,
+	xfs_dqid_t		uid,
+	xfs_dqid_t		gid,
 	prid_t			prid,
 	uint			flags,
 	struct xfs_dquot	**O_udqpp,

@@ -1697,7 +1697,7 @@ xfs_qm_vop_dqalloc(
 			 * holding ilock.
 			 */
 			xfs_iunlock(ip, lockflags);
-			if ((error = xfs_qm_dqget(mp, NULL,
 (xfs_dqid_t) uid,
+			if ((error = xfs_qm_dqget(mp, NULL, uid,
 						 XFS_DQ_USER,
 						 XFS_QMOPT_DQALLOC

| XFS_QMOPT_DOWARN,

fs/xfs/xfs_qm.c +1 -1

@@ -1723,7 +1723,7 @@ xfs_qm_vop_dqalloc(
 	if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
 		if (ip->i_d.di_gid != gid) {
 			xfs_iunlock(ip, lockflags);
-			if ((error = xfs_qm_dqget(mp, NULL,
 (xfs_dqid_t)gid,
+			if ((error = xfs_qm_dqget(mp, NULL, gid,
 						 XFS_DQ_GROUP,
 						 XFS_QMOPT_DQALLOC

| XFS_QMOPT_DOWARN,

fs/xfs/xfs_qm.c +1 -1

@@ -1842,7 +1842,7 @@ xfs_qm_vop_chown_reserve(
 			XFS_QMOPT_RES_RTBLKS :
 XFS_QMOPT_RES_REGBLKS; 
 	if (XFS_IS_UQUOTA_ON(mp) && udqp &&
-	    ip->i_d.di_uid !=
 (uid_t)be32_to_cpu(udqp->q_core.d_id)) {
+	    ip->i_d.di_uid != be32_to_cpu(udqp->q_core.d_id)) {
 		delblksudq = udqp;
 		/*

* If there are delayed allocation blocks, then we
have to diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h index c38068f..5f0bfe8 100644

fs/xfs/xfs_quota.h +0 -0

@@ -320,8 +320,8 @@ extern int

xfs_trans_reserve_quota_bydquots(struct xfs_trans *, struct xfs_mount *, struct xfs_dquot *, struct xfs_dquot *, long, long, uint);
-extern int xfs_qm_vop_dqalloc(struct xfs_inode *, uid_t, gid_t, prid_t, uint,

fs/xfs/xfs_quota.h +1 -1

-		struct xfs_dquot **, struct xfs_dquot **);
+extern int xfs_qm_vop_dqalloc(struct xfs_inode *, xfs_dqid_t,

xfs_dqid_t,

fs/xfs/xfs_quota.h +1 -0

+		prid_t, uint, struct xfs_dquot **, struct

xfs_dquot **); extern void xfs_qm_vop_create_dqattach(struct xfs_trans *, struct xfs_inode *, struct xfs_dquot *, struct xfs_dquot *); extern int xfs_qm_vop_rename_dqattach(struct xfs_inode **); @@ -341,8 +341,9 @@ extern void xfs_qm_unmount_quotas(struct xfs_mount *);
#else
static inline int
-xfs_qm_vop_dqalloc(struct xfs_inode *ip, uid_t uid, gid_t gid, prid_t prid,

fs/xfs/xfs_quota.h +0 -1

-		uint flags, struct xfs_dquot **udqp, struct

xfs_dquot **gdqp) +xfs_qm_vop_dqalloc(struct xfs_inode *ip, xfs_dqid_t uid, xfs_dqid_t gid,

fs/xfs/xfs_quota.h +2 -0

+		prid_t prid, uint flags, struct xfs_dquot **udqp,
+		struct xfs_dquot **gdqp)
 {
 	*udqp = NULL;
 	*gdqp = NULL;

fs/xfs/xfs_symlink.c +3 -1

index 195a403..c50306e 100644
@@ -384,7 +384,9 @@ xfs_symlink(
 	/*
 	 * Make sure that we have allocated dquot(s) on disk.
 	 */
-	error = xfs_qm_vop_dqalloc(dp, current_fsuid(),
 current_fsgid(), prid,
+	error = xfs_qm_vop_dqalloc(dp,
+			xfs_kuid_to_disk(current_fsuid()),
+			xfs_kgid_to_disk(current_fsgid()), prid,
 			XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
 &udqp, &gdqp); if (error)

fs/xfs/xfs_symlink.c +0 -0

 		goto std_return;

fs/xfs/xfs_vnodeops.c +3 -1

index 0176bb2..94f4f9f6 100644
@@ -515,7 +515,9 @@ xfs_create(
 	/*
 	 * Make sure that we have allocated dquot(s) on disk.
 	 */
-	error = xfs_qm_vop_dqalloc(dp, current_fsuid(),
 current_fsgid(), prid,
+	error = xfs_qm_vop_dqalloc(dp,
+			xfs_kuid_to_disk(current_fsuid()),
+			xfs_kgid_to_disk(current_fsgid()), prid,
 			XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
 &udqp, &gdqp); if (error)

fs/xfs/xfs_vnodeops.c +0 -0

 		return error;

init/Kconfig +0 -3

index 9d3a788..8083ffd 100644
@@ -1065,7 +1065,6 @@ config IPC_NS
 
 config USER_NS
 	bool "User namespace"
-	depends on UIDGID_CONVERTED
 	select UIDGID_STRICT_TYPE_CHECKS
 
 	default n

@@ -1099,21 +1098,9 @@ config NET_NS
 
 endif # NAMESPACES
 
-config UIDGID_CONVERTED
-	# True if all of the selected software conmponents are

known

init/Kconfig +0 -9

-	# to have uid_t and gid_t converted to kuid_t and kgid_t
-	# where appropriate and are otherwise safe to use with
-	# the user namespace.
-	bool
-	default y
-
-	# Filesystems
-	depends on XFS_FS = n
-
 config UIDGID_STRICT_TYPE_CHECKS
 	bool "Require conversions between uid/gids and their

internal representation"

Quoted 8 lines

init/Kconfig +1 -2

-	depends on UIDGID_CONVERTED
-	default n
+	default y
 	help
 	 While the nececessary conversions are being added to all

subsystems this option allows the code to continue to build for unconverted subsystems.

Quoted 1 line · On Tue, 25 Jun 2013 12:46:19 -0400 Brian Foster <[email protected]> wrote:

On 06/24/2013 09:10 AM, Dwight Engen wrote:

Quoted 23 lines

Quoted 23 lines

fs/xfs/xfs_icache.c +0 -0

index 96e344e..2c35b13 100644
@@ -617,7 +617,7 @@ restart:
 
 /*
  * Background scanning to trim post-EOF preallocated space. This

is queued
- * based on the 'background_prealloc_discard_period' tunable (5m by default).
+ * based on the 'speculative_prealloc_lifetime' tunable (5m by default). */
STATIC void
xfs_queue_eofblocks(

fs/xfs/xfs_icache.c +1 -1

@@ -1202,11 +1202,11 @@ xfs_inode_match_id(
 	struct xfs_eofblocks	*eofb)
 {
 	if (eofb->eof_flags & XFS_EOF_FLAGS_UID &&
-	    ip->i_d.di_uid != eofb->eof_uid)
+	    !uid_eq(VFS_I(ip)->i_uid, eofb->eof_uid))
 		return 0;
 

Quoted 7 lines

fs/xfs/xfs_icache.c xfs_inode_match_id( · line 1209

Quoted 3 lines

fs/xfs/xfs_icache.c +1 -1

 	if (eofb->eof_flags & XFS_EOF_FLAGS_GID &&
-	    ip->i_d.di_gid != eofb->eof_gid)
+	    !gid_eq(VFS_I(ip)->i_gid, eofb->eof_gid))

More of a question... since we're originally comparing against the on-disk values, is the separate internal structure strictly necessary for making eofblocks userns aware?

Not sure I fully understand your question, we were comparing on-disk uid/gid values to unconverted eof values because xfs didn't support the eof ioctl callers passing in ids from a userns. I believe part of the idea of userns is that i_uid is an opaque type, hence the use of _eq() comparators and why we have to convert eof_[ug]id if we want to compare them to i_uid as opposed to di_uid.

Quoted 9 lines

This should probably go into xfs_icache.h along with the aforementioned conversion helper.

As I mentioned previously, I have some code around that creates an internal version of the eofblocks structure. The main differences are the name (xfs_eofblocks_internal) and I did the conversion down in xfs_icache.c since I wasn't changing anything that affected the ioctl().

Quoted 2 lines

I can throw it up on the list for reference or if it's of any use as a base for this work...

If you have time to put it up that'd be great, but no biggie if not I'll write a conversion routine. Thanks for looking at this.

Quoted 145 lines

Brian

Quoted 143 lines

fs/xfs/xfs_icache.c +0 -0

 /*
  * Various platform dependent calls that don't fit anywhere else
  */

fs/xfs/xfs_qm.c +3 -3

index b75c9bb..57e2c18 100644
@@ -1651,8 +1651,8 @@ xfs_qm_write_sb_changes(
 int
 xfs_qm_vop_dqalloc(
 	struct xfs_inode	*ip,
-	uid_t			uid,
-	gid_t			gid,
+	xfs_dqid_t		uid,
+	xfs_dqid_t		gid,
 	prid_t			prid,
 	uint			flags,
 	struct xfs_dquot	**O_udqpp,

@@ -1697,7 +1697,7 @@ xfs_qm_vop_dqalloc(
 			 * holding ilock.
 			 */
 			xfs_iunlock(ip, lockflags);
-			if ((error = xfs_qm_dqget(mp, NULL,
 (xfs_dqid_t) uid,
+			if ((error = xfs_qm_dqget(mp, NULL, uid,
 						 XFS_DQ_USER,
 						 XFS_QMOPT_DQALLOC

| XFS_QMOPT_DOWARN,

fs/xfs/xfs_qm.c +1 -1

@@ -1723,7 +1723,7 @@ xfs_qm_vop_dqalloc(
 	if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
 		if (ip->i_d.di_gid != gid) {
 			xfs_iunlock(ip, lockflags);
-			if ((error = xfs_qm_dqget(mp, NULL,
 (xfs_dqid_t)gid,
+			if ((error = xfs_qm_dqget(mp, NULL, gid,
 						 XFS_DQ_GROUP,
 						 XFS_QMOPT_DQALLOC

| XFS_QMOPT_DOWARN,

fs/xfs/xfs_qm.c +1 -1

@@ -1842,7 +1842,7 @@ xfs_qm_vop_chown_reserve(
 			XFS_QMOPT_RES_RTBLKS :
 XFS_QMOPT_RES_REGBLKS; 
 	if (XFS_IS_UQUOTA_ON(mp) && udqp &&
-	    ip->i_d.di_uid !=
 (uid_t)be32_to_cpu(udqp->q_core.d_id)) {
+	    ip->i_d.di_uid != be32_to_cpu(udqp->q_core.d_id)) {
 		delblksudq = udqp;
 		/*

* If there are delayed allocation blocks, then we
have to diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h index c38068f..5f0bfe8 100644

fs/xfs/xfs_quota.h +0 -0

@@ -320,8 +320,8 @@ extern int

xfs_trans_reserve_quota_bydquots(struct xfs_trans *, struct xfs_mount *, struct xfs_dquot *, struct xfs_dquot *, long, long, uint);
-extern int xfs_qm_vop_dqalloc(struct xfs_inode *, uid_t, gid_t, prid_t, uint,

fs/xfs/xfs_quota.h +1 -1

-		struct xfs_dquot **, struct xfs_dquot **);
+extern int xfs_qm_vop_dqalloc(struct xfs_inode *, xfs_dqid_t,

xfs_dqid_t,

fs/xfs/xfs_quota.h +1 -0

+		prid_t, uint, struct xfs_dquot **, struct

xfs_dquot **); extern void xfs_qm_vop_create_dqattach(struct xfs_trans *, struct xfs_inode *, struct xfs_dquot *, struct xfs_dquot *); extern int xfs_qm_vop_rename_dqattach(struct xfs_inode **); @@ -341,8 +341,9 @@ extern void xfs_qm_unmount_quotas(struct xfs_mount *);
#else
static inline int
-xfs_qm_vop_dqalloc(struct xfs_inode *ip, uid_t uid, gid_t gid, prid_t prid,

fs/xfs/xfs_quota.h +0 -1

-		uint flags, struct xfs_dquot **udqp, struct

xfs_dquot **gdqp) +xfs_qm_vop_dqalloc(struct xfs_inode *ip, xfs_dqid_t uid, xfs_dqid_t gid,

fs/xfs/xfs_quota.h +2 -0

+		prid_t prid, uint flags, struct xfs_dquot **udqp,
+		struct xfs_dquot **gdqp)
 {
 	*udqp = NULL;
 	*gdqp = NULL;

fs/xfs/xfs_symlink.c +3 -1

index 195a403..c50306e 100644
@@ -384,7 +384,9 @@ xfs_symlink(
 	/*
 	 * Make sure that we have allocated dquot(s) on disk.
 	 */
-	error = xfs_qm_vop_dqalloc(dp, current_fsuid(),
 current_fsgid(), prid,
+	error = xfs_qm_vop_dqalloc(dp,
+			xfs_kuid_to_disk(current_fsuid()),
+			xfs_kgid_to_disk(current_fsgid()), prid,
 			XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
 &udqp, &gdqp); if (error)

fs/xfs/xfs_symlink.c +0 -0

 		goto std_return;

fs/xfs/xfs_vnodeops.c +3 -1

index 0176bb2..94f4f9f6 100644
@@ -515,7 +515,9 @@ xfs_create(
 	/*
 	 * Make sure that we have allocated dquot(s) on disk.
 	 */
-	error = xfs_qm_vop_dqalloc(dp, current_fsuid(),
 current_fsgid(), prid,
+	error = xfs_qm_vop_dqalloc(dp,
+			xfs_kuid_to_disk(current_fsuid()),
+			xfs_kgid_to_disk(current_fsgid()), prid,
 			XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
 &udqp, &gdqp); if (error)

fs/xfs/xfs_vnodeops.c +0 -0

 		return error;

init/Kconfig +0 -3

index 9d3a788..8083ffd 100644
@@ -1065,7 +1065,6 @@ config IPC_NS
 
 config USER_NS
 	bool "User namespace"
-	depends on UIDGID_CONVERTED
 	select UIDGID_STRICT_TYPE_CHECKS
 
 	default n

@@ -1099,21 +1098,9 @@ config NET_NS
 
 endif # NAMESPACES
 
-config UIDGID_CONVERTED
-	# True if all of the selected software conmponents are

known

init/Kconfig +0 -9

-	# to have uid_t and gid_t converted to kuid_t and kgid_t
-	# where appropriate and are otherwise safe to use with
-	# the user namespace.
-	bool
-	default y
-
-	# Filesystems
-	depends on XFS_FS = n
-
 config UIDGID_STRICT_TYPE_CHECKS
 	bool "Require conversions between uid/gids and their

internal representation"

init/Kconfig +0 -1

-	depends on UIDGID_CONVERTED

Quoted 8 lines

Quoted 7 lines

init/Kconfig +1 -1

-	default n
+	default y
 	help
 	 While the nececessary conversions are being added to all

subsystems this option allows the code to continue to build for unconverted subsystems.

Hi Dave, all. Here is a v2 patch that I believe addresses the previous comments, but I expect there to be more :) I think there are a few more issues to sort out before this is ready, and I want to add some tests to xfstests also.

I added permission checks for eofblocks in the ioctl code, but I don't think they are enough. Just because an unprivileged caller is in a group doesn't mean he can write to a file of that group, and I don't know how we can check for that till we get the inode in hand. Brian, if you or anyone else could comment on how this should work for the regular user and write() ENOSPC cases that'd be great.

The xfs code now uses inode->i_uid where possible instead of di_uid. The remaining uses of di_uid are where the inode is being setup, conversion to/from disk endianess, in dealing with quotas, and bulkstat.

We do need to decide on the di_uid that comes back from bulkstat. Right now it is returning on disk (== init_user_ns) uids. It looks to me like xfsrestore is using the normal vfs routines (chown, fchown, lchown) when restoring so that won't line up if the xfsrestore is run in !init_user_ns.

We could possibly convert to userns values
before returning them from the kernel, but I doubt that will work well with the xfs quotas.

Should we just require that callers of bulkstat
be in init_user_ns? Thoughts?