[PATCH review 00/16] userns: Completion of kuid/kgid/kprojid pushdown.

[PATCH review 00/16] userns: Completion of kuid/kgid/kprojid pushdown.

[Eric W. Biederman]

I have spent some more time with the xfs code and finally figured out
what needed to be done to safely add kuid/kgid support without
significant logic changes.

My first two xfs changes make the small logical adjustments required of
xfs to get the primary inode uid, gid, and projid fields that xfs uses
not live in struct xfs_icdinode.  After that the patches are pretty much
what I have posted before just split up and cleaned up so it is much
easier to understand the patches and see that they are doing reasonable
things.

xfs being the last of the filesystems to need the userns compatibility
code I my patches to remove that compatibility code are at the end of
this patchset.

Eric W. Biederman (16):
      xfs: Convert uids and gids in xfs acls to/from kuids and kgids
      xfs: Store projectid as a single variable.
      xfs: Always read uids and gids from the vfs inode
      xfs: Update inode uids, gids, and projids to be kuids, kgids, and kprojids
      xfs: Update xfs_ioctl_setattr to handle projids in any user namespace
      xfs: Use kuids and kgids in xfs_setattr_nonsize
      xfs: Update ioctl(XFS_IOC_FREE_EOFBLOCKS) to handle callers in any userspace
      xfs: Use kprojids when allocating inodes.
      xfs: Modify xfs_qm_vop_dqalloc to take kuids, kgids, and kprojids.
      xfs: Push struct kqid into xfs_qm_scall_qmlim and xfs_qm_scall_getquota
      xfs: Modify xfs_qm_dqget to take a struct kqid.
      xfs: Remember the kqid for a quota
      xfs: Use q_id instead of q_core.d_id.
      xfs: Enable building with user namespaces enabled.
      userns: Now that everything has been converted remove the unnecessary infrastructure
      userns: Remove the EXPERMINTAL kconfig tag

 fs/xfs/xfs_acl.c          |   23 ++++++++++--
 fs/xfs/xfs_dquot.c        |   39 ++++++++++++++-------
 fs/xfs/xfs_dquot.h        |   13 +++++--
 fs/xfs/xfs_icache.c       |   12 +++---
 fs/xfs/xfs_icache.h       |   11 +++++-
 fs/xfs/xfs_inode.c        |   25 +++++++++----
 fs/xfs/xfs_inode.h        |   24 +++++++++----
 fs/xfs/xfs_ioctl.c        |   54 ++++++++++++++++++++++------
 fs/xfs/xfs_iops.c         |   34 ++++++++----------
 fs/xfs/xfs_itable.c       |   10 +++--
 fs/xfs/xfs_qm.c           |   85 ++++++++++++++++++++++-----------------------
 fs/xfs/xfs_qm.h           |    4 +-
 fs/xfs/xfs_qm_bhv.c       |    2 +-
 fs/xfs/xfs_qm_syscalls.c  |   24 +++++++------
 fs/xfs/xfs_quota.h        |    4 +-
 fs/xfs/xfs_quotaops.c     |   20 +---------
 fs/xfs/xfs_rename.c       |    2 +-
 fs/xfs/xfs_trace.h        |    2 +-
 fs/xfs/xfs_trans_dquot.c  |    8 +---
 fs/xfs/xfs_utils.c        |    2 +-
 fs/xfs/xfs_utils.h        |    2 +-
 fs/xfs/xfs_vnodeops.c     |   14 ++++----
 include/linux/posix_acl.h |    3 --
 include/linux/projid.h    |   15 --------
 include/linux/uidgid.h    |   22 ------------
 init/Kconfig              |   27 +--------------
 26 files changed, 244 insertions(+), 237 deletions(-)

[PATCH review 01/16] xfs: Convert uids and gids in xfs acls to/from kuids and kgids

[Eric W. Biederman]

From: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>

- When reading from disk convert on disk uids and gids to kuids and kgids
- When writing to the disk convert in memory kuids and kgids to uids and gids.
- Don't write e_id as that field only exists when user namespace
  support is disabled.
- Use uid_eq when testing to see if current_fsuid() is allowed to set the
  acls for a file.

Cc: Ben Myers <bpm-sJ/iWh9BUns@public.gmane.org>
Cc: Alex Elder <elder-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org>
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
 fs/xfs/xfs_acl.c |   23 +++++++++++++++++++----
 1 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
index 1d32f1d..ca2aade 100644
--- a/fs/xfs/xfs_acl.c
+++ b/fs/xfs/xfs_acl.c
@@ -64,14 +64,17 @@ xfs_acl_from_disk(struct xfs_acl *aclp)
 
 		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;
@@ -97,8 +100,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);
 		ace->ae_perm = cpu_to_be16(acl_e->e_perm);
+		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;
+		}
 	}
 }
 
@@ -355,7 +370,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 ((!uid_eq(current_fsuid(), inode->i_uid)) && !capable(CAP_FOWNER))
 		return -EPERM;
 
 	if (!value)
-- 
1.7.5.4

[PATCH review 02/16] xfs: Store projectid as a single variable.

[Eric W. Biederman]

From: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>

xfs_get_projid is torturous to read and it will not work at all when
project ids are stored in a kprojid_t.  So add a i_projid to
xfs_inode, that is cheap to read and can handle future needs, and
update all callers of xfs_get_projid to use i_projid.

Retain xfs_set_projid to handle the needed double updates, as there
are now two places the value needs to be set.

In xfs_iread after filling in i_d drom the on-disk inode update the
new i_projid field.

Cc: Ben Myers <bpm-sJ/iWh9BUns@public.gmane.org>
Cc: Alex Elder <elder-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org>
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
 fs/xfs/xfs_icache.c   |    2 +-
 fs/xfs/xfs_inode.c    |    6 +++++-
 fs/xfs/xfs_inode.h    |    7 ++-----
 fs/xfs/xfs_ioctl.c    |    6 +++---
 fs/xfs/xfs_iops.c     |    2 +-
 fs/xfs/xfs_itable.c   |    4 ++--
 fs/xfs/xfs_qm.c       |   10 +++++-----
 fs/xfs/xfs_qm_bhv.c   |    2 +-
 fs/xfs/xfs_rename.c   |    2 +-
 fs/xfs/xfs_vnodeops.c |    6 +++---
 10 files changed, 24 insertions(+), 23 deletions(-)

diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 96e344e..4f109ca 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -1210,7 +1210,7 @@ xfs_inode_match_id(
 		return 0;
 
 	if (eofb->eof_flags & XFS_EOF_FLAGS_PRID &&
-	    xfs_get_projid(ip) != eofb->eof_prid)
+	    ip->i_projid != eofb->eof_prid)
 		return 0;
 
 	return 1;
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 66282dc..51c2597 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1013,6 +1013,10 @@ xfs_iread(
 	 */
 	if (dip->di_mode) {
 		xfs_dinode_from_disk(&ip->i_d, dip);
+
+		ip->i_projid = ((projid_t)ip->i_d.di_projid_hi << 16) |
+					  ip->i_d.di_projid_lo;
+
 		error = xfs_iformat(ip, dip);
 		if (error)  {
 #ifdef DEBUG
@@ -2839,7 +2843,7 @@ xfs_iflush_int(
 			memset(&(ip->i_d.di_pad[0]), 0, sizeof(ip->i_d.di_pad));
 			memset(&(dip->di_pad[0]), 0,
 			      sizeof(dip->di_pad));
-			ASSERT(xfs_get_projid(ip) == 0);
+			ASSERT(ip->i_projid == 0);
 		}
 	}
 
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index 22baf6e..b9e4389 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -244,6 +244,7 @@ typedef struct xfs_inode {
 	unsigned int		i_delayed_blks;	/* count of delay alloc blks */
 
 	xfs_icdinode_t		i_d;		/* most of ondisk inode */
+	projid_t		i_projid;	/* Project id */
 
 	/* VFS inode */
 	struct inode		i_vnode;	/* embedded VFS inode */
@@ -359,16 +360,12 @@ xfs_iflags_test_and_set(xfs_inode_t *ip, unsigned short flags)
  * and using two 16bit values to hold new 32bit projid was chosen
  * to retain compatibility with "old" filesystems).
  */
-static inline prid_t
-xfs_get_projid(struct xfs_inode *ip)
-{
-	return (prid_t)ip->i_d.di_projid_hi << 16 | ip->i_d.di_projid_lo;
-}
 
 static inline void
 xfs_set_projid(struct xfs_inode *ip,
 		prid_t projid)
 {
+	ip->i_projid = projid;
 	ip->i_d.di_projid_hi = (__uint16_t) (projid >> 16);
 	ip->i_d.di_projid_lo = (__uint16_t) (projid & 0xffff);
 }
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index c1c3ef8..4afb509 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -815,7 +815,7 @@ xfs_ioc_fsgetxattr(
 	xfs_ilock(ip, XFS_ILOCK_SHARED);
 	fa.fsx_xflags = xfs_ip2xflags(ip);
 	fa.fsx_extsize = ip->i_d.di_extsize << ip->i_mount->m_sb.sb_blocklog;
-	fa.fsx_projid = xfs_get_projid(ip);
+	fa.fsx_projid = ip->i_projid;
 
 	if (attr) {
 		if (ip->i_afp) {
@@ -986,7 +986,7 @@ xfs_ioctl_setattr(
 	if (mask & FSX_PROJID) {
 		if (XFS_IS_QUOTA_RUNNING(mp) &&
 		    XFS_IS_PQUOTA_ON(mp) &&
-		    xfs_get_projid(ip) != fa->fsx_projid) {
+		    ip->i_projid != fa->fsx_projid) {
 			ASSERT(tp);
 			code = xfs_qm_vop_chown_reserve(tp, ip, udqp, gdqp,
 						capable(CAP_FOWNER) ?
@@ -1104,7 +1104,7 @@ xfs_ioctl_setattr(
 		 * Change the ownerships and register quota modifications
 		 * in the transaction.
 		 */
-		if (xfs_get_projid(ip) != fa->fsx_projid) {
+		if (ip->i_projid != fa->fsx_projid) {
 			if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_PQUOTA_ON(mp)) {
 				olddquot = xfs_qm_vop_chown(tp, ip,
 							&ip->i_gdquot, gdqp);
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index d82efaa..643f55a 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -516,7 +516,7 @@ xfs_setattr_nonsize(
 		 */
 		ASSERT(udqp == NULL);
 		ASSERT(gdqp == NULL);
-		error = xfs_qm_vop_dqalloc(ip, uid, gid, xfs_get_projid(ip),
+		error = xfs_qm_vop_dqalloc(ip, uid, gid, ip->i_projid,
 					 qflags, &udqp, &gdqp);
 		if (error)
 			return error;
diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index 2ea7d40..cf5b1d0 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -91,8 +91,8 @@ xfs_bulkstat_one_int(
 	 * further change.
 	 */
 	buf->bs_nlink = dic->di_nlink;
-	buf->bs_projid_lo = dic->di_projid_lo;
-	buf->bs_projid_hi = dic->di_projid_hi;
+	buf->bs_projid_lo = (u16)(ip->i_projid & 0xffff);
+	buf->bs_projid_hi = (u16)(ip->i_projid >> 16);
 	buf->bs_ino = ino;
 	buf->bs_mode = dic->di_mode;
 	buf->bs_uid = dic->di_uid;
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 60eff47..e5f48a7 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -530,7 +530,7 @@ xfs_qm_dqattach_locked(
 			xfs_qm_dqattach_one(ip, ip->i_d.di_gid, XFS_DQ_GROUP,
 						flags & XFS_QMOPT_DQALLOC,
 						ip->i_udquot, &ip->i_gdquot) :
-			xfs_qm_dqattach_one(ip, xfs_get_projid(ip), XFS_DQ_PROJ,
+			xfs_qm_dqattach_one(ip, ip->i_projid, XFS_DQ_PROJ,
 						flags & XFS_QMOPT_DQALLOC,
 						ip->i_udquot, &ip->i_gdquot);
 		/*
@@ -1172,7 +1172,7 @@ xfs_qm_dqusage_adjust(
 	}
 
 	if (XFS_IS_PQUOTA_ON(mp)) {
-		error = xfs_qm_quotacheck_dqadjust(ip, xfs_get_projid(ip),
+		error = xfs_qm_quotacheck_dqadjust(ip, ip->i_projid,
 						   XFS_DQ_PROJ, nblks, rtblks);
 		if (error)
 			goto error0;
@@ -1705,7 +1705,7 @@ xfs_qm_vop_dqalloc(
 			gq = xfs_qm_dqhold(ip->i_gdquot);
 		}
 	} else if ((flags & XFS_QMOPT_PQUOTA) && XFS_IS_PQUOTA_ON(mp)) {
-		if (xfs_get_projid(ip) != prid) {
+		if (ip->i_projid != prid) {
 			xfs_iunlock(ip, lockflags);
 			if ((error = xfs_qm_dqget(mp, NULL, (xfs_dqid_t)prid,
 						 XFS_DQ_PROJ,
@@ -1820,7 +1820,7 @@ xfs_qm_vop_chown_reserve(
 	}
 	if (XFS_IS_OQUOTA_ON(ip->i_mount) && gdqp) {
 		if (XFS_IS_PQUOTA_ON(ip->i_mount) &&
-		     xfs_get_projid(ip) != be32_to_cpu(gdqp->q_core.d_id))
+		     ip->i_projid != be32_to_cpu(gdqp->q_core.d_id))
 			prjflags = XFS_QMOPT_ENOSPC;
 
 		if (prjflags ||
@@ -1918,7 +1918,7 @@ xfs_qm_vop_create_dqattach(
 		ASSERT(ip->i_gdquot == NULL);
 		ASSERT(XFS_IS_OQUOTA_ON(mp));
 		ASSERT((XFS_IS_GQUOTA_ON(mp) ?
-			ip->i_d.di_gid : xfs_get_projid(ip)) ==
+			ip->i_d.di_gid : ip->i_projid) ==
 				be32_to_cpu(gdqp->q_core.d_id));
 
 		ip->i_gdquot = xfs_qm_dqhold(gdqp);
diff --git a/fs/xfs/xfs_qm_bhv.c b/fs/xfs/xfs_qm_bhv.c
index 6b39115..3abac1b 100644
--- a/fs/xfs/xfs_qm_bhv.c
+++ b/fs/xfs/xfs_qm_bhv.c
@@ -79,7 +79,7 @@ xfs_qm_statvfs(
 	xfs_mount_t		*mp = ip->i_mount;
 	xfs_dquot_t		*dqp;
 
-	if (!xfs_qm_dqget(mp, NULL, xfs_get_projid(ip), XFS_DQ_PROJ, 0, &dqp)) {
+	if (!xfs_qm_dqget(mp, NULL, ip->i_projid, XFS_DQ_PROJ, 0, &dqp)) {
 		xfs_fill_statvfs_from_dquot(statp, dqp);
 		xfs_qm_dqput(dqp);
 	}
diff --git a/fs/xfs/xfs_rename.c b/fs/xfs/xfs_rename.c
index 30ff5f4..89981bb 100644
--- a/fs/xfs/xfs_rename.c
+++ b/fs/xfs/xfs_rename.c
@@ -171,7 +171,7 @@ xfs_rename(
 	 * tree quota mechanism would be circumvented.
 	 */
 	if (unlikely((target_dp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT) &&
-		     (xfs_get_projid(target_dp) != xfs_get_projid(src_ip)))) {
+		     (target_dp->i_projid != src_ip->i_projid))) {
 		error = XFS_ERROR(EXDEV);
 		goto error_return;
 	}
diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
index d95f565..7a2c2a0 100644
--- a/fs/xfs/xfs_vnodeops.c
+++ b/fs/xfs/xfs_vnodeops.c
@@ -741,7 +741,7 @@ xfs_create(
 		return XFS_ERROR(EIO);
 
 	if (dp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT)
-		prid = xfs_get_projid(dp);
+		prid = dp->i_projid;
 	else
 		prid = XFS_PROJID_DEFAULT;
 
@@ -1305,7 +1305,7 @@ xfs_link(
 	 * the tree quota mechanism could be circumvented.
 	 */
 	if (unlikely((tdp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT) &&
-		     (xfs_get_projid(tdp) != xfs_get_projid(sip)))) {
+		     (tdp->i_projid != sip->i_projid))) {
 		error = XFS_ERROR(EXDEV);
 		goto error_return;
 	}
@@ -1402,7 +1402,7 @@ xfs_symlink(
 
 	udqp = gdqp = NULL;
 	if (dp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT)
-		prid = xfs_get_projid(dp);
+		prid = dp->i_projid;
 	else
 		prid = XFS_PROJID_DEFAULT;
 
-- 
1.7.5.4

Re: [PATCH review 02/16] xfs: Store projectid as a single variable.

[Dave Chinner]

On Sun, Feb 17, 2013 at 05:10:55PM -0800, Eric W. Biederman wrote:
> From: "Eric W. Biederman" <ebiederm@xmission.com>
> 
> xfs_get_projid is torturous to read and it will not work at all when
> project ids are stored in a kprojid_t.  So add a i_projid to
> xfs_inode, that is cheap to read and can handle future needs, and
> update all callers of xfs_get_projid to use i_projid.
> 
> Retain xfs_set_projid to handle the needed double updates, as there
> are now two places the value needs to be set.
> 
> In xfs_iread after filling in i_d drom the on-disk inode update the
> new i_projid field.
> 
> Cc: Ben Myers <bpm@sgi.com>
> Cc: Alex Elder <elder@kernel.org>
> Cc: Dave Chinner <david@fromorbit.com>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>  fs/xfs/xfs_icache.c   |    2 +-
>  fs/xfs/xfs_inode.c    |    6 +++++-
>  fs/xfs/xfs_inode.h    |    7 ++-----
>  fs/xfs/xfs_ioctl.c    |    6 +++---
>  fs/xfs/xfs_iops.c     |    2 +-
>  fs/xfs/xfs_itable.c   |    4 ++--
>  fs/xfs/xfs_qm.c       |   10 +++++-----
>  fs/xfs/xfs_qm_bhv.c   |    2 +-
>  fs/xfs/xfs_rename.c   |    2 +-
>  fs/xfs/xfs_vnodeops.c |    6 +++---
>  10 files changed, 24 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 96e344e..4f109ca 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -1210,7 +1210,7 @@ xfs_inode_match_id(
>  		return 0;
>  
>  	if (eofb->eof_flags & XFS_EOF_FLAGS_PRID &&
> -	    xfs_get_projid(ip) != eofb->eof_prid)
> +	    ip->i_projid != eofb->eof_prid)
>  		return 0;

Please retain the xfs_get_projid(ip) wrapper and do all the
necessary conversions via that wrapper. We don't need a second copy
of the project ID to support namespace aware project ID support.

>  	return 1;
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 66282dc..51c2597 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1013,6 +1013,10 @@ xfs_iread(
>  	 */
>  	if (dip->di_mode) {
>  		xfs_dinode_from_disk(&ip->i_d, dip);
> +
> +		ip->i_projid = ((projid_t)ip->i_d.di_projid_hi << 16) |
> +					  ip->i_d.di_projid_lo;
> +

This does not belong here. At minimum, it would need to be in
xfs_iformat(). Further, if there is a requirement it is initialised
correctly, it needs to be zeroed in xfs_inode_alloc() where we pull
a newly allocated inode out of the slab.

As it is, however, having read further through the patches, I can't
really see why we need even need a separate variable - the
conversions shoul dbe done at the edge of the filesystem (i.e. VFS
and ioctl interfaces, and the core of the filesystem left completely
untouched.

>  static inline void
>  xfs_set_projid(struct xfs_inode *ip,
>  		prid_t projid)
>  {
> +	ip->i_projid = projid;
>  	ip->i_d.di_projid_hi = (__uint16_t) (projid >> 16);
>  	ip->i_d.di_projid_lo = (__uint16_t) (projid & 0xffff);
>  }

As all you are doing is introduing a requirement that we keep two
variables in sync and increase the size of the struct xfs_inode
unnecessarily. History says this sort of duplication is a source of
subtle bugs....

> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index 2ea7d40..cf5b1d0 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -91,8 +91,8 @@ xfs_bulkstat_one_int(
>  	 * further change.
>  	 */
>  	buf->bs_nlink = dic->di_nlink;
> -	buf->bs_projid_lo = dic->di_projid_lo;
> -	buf->bs_projid_hi = dic->di_projid_hi;
> +	buf->bs_projid_lo = (u16)(ip->i_projid & 0xffff);
> +	buf->bs_projid_hi = (u16)(ip->i_projid >> 16);

There is no need for this change at all. Even if we have a second
variable, the two are in sync and of reading from the dic is
perfectly OK.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH review 03/16] xfs: Always read uids and gids from the vfs inode

[Eric W. Biederman]

From: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>

Add xfs_set_uid and xfs_set_gid to encapsulate the double write
needed when updating uid and gids, and uset them for all uid
and gid writes.

Update VFS()->i_uid and VFS_I()->i_gid immediately after reading
on-disk inode values so that all of the cached uid and gid values
are always in sync allowing VFS()->i_uid and VFS()->i_gid to safely
be used everywhere.

Replace reads of i_d.di_uid and i_d.di_gid with VFS_I()->i_uid and
VFS_I()->i_gid.

This is a precursor to pushing kuids and kgids down into xfs.

Cc: Ben Myers <bpm-sJ/iWh9BUns@public.gmane.org>
Cc: Alex Elder <elder-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org>
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
 fs/xfs/xfs_icache.c |    4 ++--
 fs/xfs/xfs_inode.c  |   10 ++++++----
 fs/xfs/xfs_inode.h  |   12 ++++++++++++
 fs/xfs/xfs_ioctl.c  |    6 +++---
 fs/xfs/xfs_iops.c   |   20 ++++++++------------
 fs/xfs/xfs_itable.c |    4 ++--
 fs/xfs/xfs_qm.c     |   22 +++++++++++-----------
 7 files changed, 44 insertions(+), 34 deletions(-)

diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 4f109ca..eba1dbd 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -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)
+	    VFS_I(ip)->i_uid != eofb->eof_uid)
 		return 0;
 
 	if (eofb->eof_flags & XFS_EOF_FLAGS_GID &&
-	    ip->i_d.di_gid != eofb->eof_gid)
+	    VFS_I(ip)->i_gid != eofb->eof_gid)
 		return 0;
 
 	if (eofb->eof_flags & XFS_EOF_FLAGS_PRID &&
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 51c2597..846166d 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1016,6 +1016,8 @@ xfs_iread(
 
 		ip->i_projid = ((projid_t)ip->i_d.di_projid_hi << 16) |
 					  ip->i_d.di_projid_lo;
+		VFS_I(ip)->i_uid = ip->i_d.di_uid;
+		VFS_I(ip)->i_gid = ip->i_d.di_gid;
 
 		error = xfs_iformat(ip, dip);
 		if (error)  {
@@ -1201,8 +1203,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();
+	xfs_set_uid(ip, current_fsuid());
+	xfs_set_gid(ip, current_fsgid());
 	xfs_set_projid(ip, prid);
 	memset(&(ip->i_d.di_pad[0]), 0, sizeof(ip->i_d.di_pad));
 
@@ -1228,7 +1230,7 @@ xfs_ialloc(
 		xfs_bump_ino_vers2(tp, ip);
 
 	if (pip && XFS_INHERIT_GID(pip)) {
-		ip->i_d.di_gid = pip->i_d.di_gid;
+		xfs_set_gid(ip, VFS_I(pip)->i_gid);
 		if ((pip->i_d.di_mode & S_ISGID) && S_ISDIR(mode)) {
 			ip->i_d.di_mode |= S_ISGID;
 		}
@@ -1241,7 +1243,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(VFS_I(ip)->i_gid))) {
 		ip->i_d.di_mode &= ~S_ISGID;
 	}
 
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index b9e4389..f503aed 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -370,6 +370,18 @@ xfs_set_projid(struct xfs_inode *ip,
 	ip->i_d.di_projid_lo = (__uint16_t) (projid & 0xffff);
 }
 
+static inline void xfs_set_uid(struct xfs_inode *ip, uid_t uid)
+{
+	VFS_I(ip)->i_uid = uid;
+	ip->i_d.di_uid = uid;
+}
+
+static inline void xfs_set_gid(struct xfs_inode *ip, gid_t gid)
+{
+	VFS_I(ip)->i_gid = gid;
+	ip->i_d.di_gid = gid;
+}
+
 /*
  * In-core inode flags.
  */
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 4afb509..db274d4 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -949,8 +949,8 @@ xfs_ioctl_setattr(
 	 * because the i_*dquot fields will get updated anyway.
 	 */
 	if (XFS_IS_QUOTA_ON(mp) && (mask & FSX_PROJID)) {
-		code = xfs_qm_vop_dqalloc(ip, ip->i_d.di_uid,
-					 ip->i_d.di_gid, fa->fsx_projid,
+		code = xfs_qm_vop_dqalloc(ip, VFS_I(ip)->i_uid,
+					 VFS_I(ip)->i_gid, fa->fsx_projid,
 					 XFS_QMOPT_PQUOTA, &udqp, &gdqp);
 		if (code)
 			return code;
@@ -975,7 +975,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 (current_fsuid() != VFS_I(ip)->i_uid && !capable(CAP_FOWNER)) {
 		code = XFS_ERROR(EPERM);
 		goto error_return;
 	}
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 643f55a..235a48c 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -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 = VFS_I(ip)->i_uid;
+	stat->gid = VFS_I(ip)->i_gid;
 	stat->ino = ip->i_ino;
 	stat->atime = inode->i_atime;
 	stat->mtime = inode->i_mtime;
@@ -500,13 +500,13 @@ xfs_setattr_nonsize(
 			uid = iattr->ia_uid;
 			qflags |= XFS_QMOPT_UQUOTA;
 		} else {
-			uid = ip->i_d.di_uid;
+			uid = VFS_I(ip)->i_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 = VFS_I(ip)->i_gid;
 		}
 
 		/*
@@ -539,8 +539,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 = VFS_I(ip)->i_uid;
+		igid = VFS_I(ip)->i_gid;
 		gid = (mask & ATTR_GID) ? iattr->ia_gid : igid;
 		uid = (mask & ATTR_UID) ? iattr->ia_uid : iuid;
 
@@ -587,8 +587,7 @@ xfs_setattr_nonsize(
 				olddquot1 = xfs_qm_vop_chown(tp, ip,
 							&ip->i_udquot, udqp);
 			}
-			ip->i_d.di_uid = uid;
-			inode->i_uid = uid;
+			xfs_set_uid(ip, uid);
 		}
 		if (igid != gid) {
 			if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_GQUOTA_ON(mp)) {
@@ -598,8 +597,7 @@ xfs_setattr_nonsize(
 				olddquot2 = xfs_qm_vop_chown(tp, ip,
 							&ip->i_gdquot, gdqp);
 			}
-			ip->i_d.di_gid = gid;
-			inode->i_gid = gid;
+			xfs_set_gid(ip, gid);
 		}
 	}
 
@@ -1155,8 +1153,6 @@ 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;
 
 	switch (inode->i_mode & S_IFMT) {
 	case S_IFBLK:
diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index cf5b1d0..a9e07dd 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -95,8 +95,8 @@ xfs_bulkstat_one_int(
 	buf->bs_projid_hi = (u16)(ip->i_projid >> 16);
 	buf->bs_ino = ino;
 	buf->bs_mode = dic->di_mode;
-	buf->bs_uid = dic->di_uid;
-	buf->bs_gid = dic->di_gid;
+	buf->bs_uid = VFS_I(ip)->i_uid;
+	buf->bs_gid = VFS_I(ip)->i_gid;
 	buf->bs_size = dic->di_size;
 	buf->bs_atime.tv_sec = dic->di_atime.t_sec;
 	buf->bs_atime.tv_nsec = dic->di_atime.t_nsec;
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index e5f48a7..c1310ed 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -516,7 +516,7 @@ xfs_qm_dqattach_locked(
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 
 	if (XFS_IS_UQUOTA_ON(mp)) {
-		error = xfs_qm_dqattach_one(ip, ip->i_d.di_uid, XFS_DQ_USER,
+		error = xfs_qm_dqattach_one(ip, VFS_I(ip)->i_uid, XFS_DQ_USER,
 						flags & XFS_QMOPT_DQALLOC,
 						NULL, &ip->i_udquot);
 		if (error)
@@ -527,7 +527,7 @@ xfs_qm_dqattach_locked(
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 	if (XFS_IS_OQUOTA_ON(mp)) {
 		error = XFS_IS_GQUOTA_ON(mp) ?
-			xfs_qm_dqattach_one(ip, ip->i_d.di_gid, XFS_DQ_GROUP,
+			xfs_qm_dqattach_one(ip, VFS_I(ip)->i_gid, XFS_DQ_GROUP,
 						flags & XFS_QMOPT_DQALLOC,
 						ip->i_udquot, &ip->i_gdquot) :
 			xfs_qm_dqattach_one(ip, ip->i_projid, XFS_DQ_PROJ,
@@ -1158,14 +1158,14 @@ xfs_qm_dqusage_adjust(
 	 * and quotaoffs don't race. (Quotachecks happen at mount time only).
 	 */
 	if (XFS_IS_UQUOTA_ON(mp)) {
-		error = xfs_qm_quotacheck_dqadjust(ip, ip->i_d.di_uid,
+		error = xfs_qm_quotacheck_dqadjust(ip, VFS_I(ip)->i_uid,
 						   XFS_DQ_USER, nblks, rtblks);
 		if (error)
 			goto error0;
 	}
 
 	if (XFS_IS_GQUOTA_ON(mp)) {
-		error = xfs_qm_quotacheck_dqadjust(ip, ip->i_d.di_gid,
+		error = xfs_qm_quotacheck_dqadjust(ip, VFS_I(ip)->i_gid,
 						   XFS_DQ_GROUP, nblks, rtblks);
 		if (error)
 			goto error0;
@@ -1634,7 +1634,7 @@ xfs_qm_vop_dqalloc(
 	xfs_ilock(ip, lockflags);
 
 	if ((flags & XFS_QMOPT_INHERIT) && XFS_INHERIT_GID(ip))
-		gid = ip->i_d.di_gid;
+		gid = VFS_I(ip)->i_gid;
 
 	/*
 	 * Attach the dquot(s) to this inode, doing a dquot allocation
@@ -1650,7 +1650,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 (VFS_I(ip)->i_uid != uid) {
 			/*
 			 * What we need is the dquot that has this uid, and
 			 * if we send the inode to dqget, the uid of the inode
@@ -1685,7 +1685,7 @@ xfs_qm_vop_dqalloc(
 		}
 	}
 	if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
-		if (ip->i_d.di_gid != gid) {
+		if (VFS_I(ip)->i_gid != gid) {
 			xfs_iunlock(ip, lockflags);
 			if ((error = xfs_qm_dqget(mp, NULL, (xfs_dqid_t)gid,
 						 XFS_DQ_GROUP,
@@ -1806,7 +1806,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)) {
+	    VFS_I(ip)->i_uid != (uid_t)be32_to_cpu(udqp->q_core.d_id)) {
 		delblksudq = udqp;
 		/*
 		 * If there are delayed allocation blocks, then we have to
@@ -1825,7 +1825,7 @@ xfs_qm_vop_chown_reserve(
 
 		if (prjflags ||
 		    (XFS_IS_GQUOTA_ON(ip->i_mount) &&
-		     ip->i_d.di_gid != be32_to_cpu(gdqp->q_core.d_id))) {
+		     VFS_I(ip)->i_gid != be32_to_cpu(gdqp->q_core.d_id))) {
 			delblksgdq = gdqp;
 			if (delblks) {
 				ASSERT(ip->i_gdquot);
@@ -1909,7 +1909,7 @@ xfs_qm_vop_create_dqattach(
 	if (udqp) {
 		ASSERT(ip->i_udquot == NULL);
 		ASSERT(XFS_IS_UQUOTA_ON(mp));
-		ASSERT(ip->i_d.di_uid == be32_to_cpu(udqp->q_core.d_id));
+		ASSERT(VFS_I(ip)->i_uid == be32_to_cpu(udqp->q_core.d_id));
 
 		ip->i_udquot = xfs_qm_dqhold(udqp);
 		xfs_trans_mod_dquot(tp, udqp, XFS_TRANS_DQ_ICOUNT, 1);
@@ -1918,7 +1918,7 @@ xfs_qm_vop_create_dqattach(
 		ASSERT(ip->i_gdquot == NULL);
 		ASSERT(XFS_IS_OQUOTA_ON(mp));
 		ASSERT((XFS_IS_GQUOTA_ON(mp) ?
-			ip->i_d.di_gid : ip->i_projid) ==
+			VFS_I(ip)->i_gid : ip->i_projid) ==
 				be32_to_cpu(gdqp->q_core.d_id));
 
 		ip->i_gdquot = xfs_qm_dqhold(gdqp);
-- 
1.7.5.4

Re: [PATCH review 03/16] xfs: Always read uids and gids from the vfs inode

[Dave Chinner]

On Sun, Feb 17, 2013 at 05:10:56PM -0800, Eric W. Biederman wrote:
> From: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
> 
> Add xfs_set_uid and xfs_set_gid to encapsulate the double write
> needed when updating uid and gids, and uset them for all uid
> and gid writes.
>
> Update VFS()->i_uid and VFS_I()->i_gid immediately after reading
> on-disk inode values so that all of the cached uid and gid values
> are always in sync allowing VFS()->i_uid and VFS()->i_gid to safely
> be used everywhere.
>
> Replace reads of i_d.di_uid and i_d.di_gid with VFS_I()->i_uid and
> VFS_I()->i_gid.

tl;dr: gross layering violation.

> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 51c2597..846166d 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1016,6 +1016,8 @@ xfs_iread(
>  
>  		ip->i_projid = ((projid_t)ip->i_d.di_projid_hi << 16) |
>  					  ip->i_d.di_projid_lo;
> +		VFS_I(ip)->i_uid = ip->i_d.di_uid;
> +		VFS_I(ip)->i_gid = ip->i_d.di_gid;

This is layers below anything VFS related and as such is a a gross
layering violation. There are many operations done in XFS on inodes
outside the life cycle of the struct inode, and so we cannot safely
use anything in the struct inode outside of those contexts.

The VFS struct inode values are only valid inside the defined life
cycle of the VFS inode, and that means from xfs_setup_inode() to
xfs_fs_evict_inode()/xfs_inactive(). Any use of uid/gid/prid outside
those boundaries is completely internal to XFS and needs to be
treated as such.

> @@ -1201,8 +1203,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();
> +	xfs_set_uid(ip, current_fsuid());
> +	xfs_set_gid(ip, current_fsgid());

Same layering violation.


>  	xfs_set_projid(ip, prid);
>  	memset(&(ip->i_d.di_pad[0]), 0, sizeof(ip->i_d.di_pad));
>  
> @@ -1228,7 +1230,7 @@ xfs_ialloc(
>  		xfs_bump_ino_vers2(tp, ip);
>  
>  	if (pip && XFS_INHERIT_GID(pip)) {
> -		ip->i_d.di_gid = pip->i_d.di_gid;
> +		xfs_set_gid(ip, VFS_I(pip)->i_gid);

NACK. This is a pure parent->child value inheritence internal to
XFS, and is way below the visibility of the VFS.

>  		if ((pip->i_d.di_mode & S_ISGID) && S_ISDIR(mode)) {
>  			ip->i_d.di_mode |= S_ISGID;
>  		}
> @@ -1241,7 +1243,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(VFS_I(ip)->i_gid))) {
>  		ip->i_d.di_mode &= ~S_ISGID;
>  	}

If this needs to be namespace aware, then convert the
ip->i_d.di_gid to the namespace structure dynamically for the call
to in_group_p().

> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 4afb509..db274d4 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -949,8 +949,8 @@ xfs_ioctl_setattr(
>  	 * because the i_*dquot fields will get updated anyway.
>  	 */
>  	if (XFS_IS_QUOTA_ON(mp) && (mask & FSX_PROJID)) {
> -		code = xfs_qm_vop_dqalloc(ip, ip->i_d.di_uid,
> -					 ip->i_d.di_gid, fa->fsx_projid,
> +		code = xfs_qm_vop_dqalloc(ip, VFS_I(ip)->i_uid,
> +					 VFS_I(ip)->i_gid, fa->fsx_projid,
>  					 XFS_QMOPT_PQUOTA, &udqp, &gdqp);

The quota code assumes a direct relationship between the values in
the struct xfs_inode and the dquot ID. It is not a relationship that
namespaces enter into. namespace conversion should happen at the
edge of the filesystem quota subsystem, (i.e. into an xfs_dqid_t)
and the rest of the code left alone.

> @@ -500,13 +500,13 @@ xfs_setattr_nonsize(
>  			uid = iattr->ia_uid;
>  			qflags |= XFS_QMOPT_UQUOTA;
>  		} else {
> -			uid = ip->i_d.di_uid;
> +			uid = VFS_I(ip)->i_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 = VFS_I(ip)->i_gid;
>  		}

Same again - quota IDs are related to the on disk inode value, not
the VFS, namespace aware value.

> @@ -539,8 +539,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 = VFS_I(ip)->i_uid;
> +		igid = VFS_I(ip)->i_gid;
>  		gid = (mask & ATTR_GID) ? iattr->ia_gid : igid;
>  		uid = (mask & ATTR_UID) ? iattr->ia_uid : iuid;
>  
> @@ -587,8 +587,7 @@ xfs_setattr_nonsize(
>  				olddquot1 = xfs_qm_vop_chown(tp, ip,
>  							&ip->i_udquot, udqp);
>  			}
> -			ip->i_d.di_uid = uid;
> -			inode->i_uid = uid;
> +			xfs_set_uid(ip, uid);

PLease keep these as separate updates, that way we can see clearly
that we are updating both the VFS inode and the XFS inode here.

> @@ -1155,8 +1153,6 @@ 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;

Which further empahsises the layer violation...

>  	switch (inode->i_mode & S_IFMT) {
>  	case S_IFBLK:
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index cf5b1d0..a9e07dd 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -95,8 +95,8 @@ xfs_bulkstat_one_int(
>  	buf->bs_projid_hi = (u16)(ip->i_projid >> 16);
>  	buf->bs_ino = ino;
>  	buf->bs_mode = dic->di_mode;
> -	buf->bs_uid = dic->di_uid;
> -	buf->bs_gid = dic->di_gid;
> +	buf->bs_uid = VFS_I(ip)->i_uid;
> +	buf->bs_gid = VFS_I(ip)->i_gid;

Same as the project ID changes - bulkstat is supposed to return the
raw on disk values, not namespace munged values.

Cheers,

Dave.
-- 
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org

[PATCH review 04/16] xfs: Update inode uids, gids, and projids to be kuids, kgids, and kprojids

[Eric W. Biederman]

From: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>

Update i_projid on struct xfs_inode to be of type  kprojid_t.

Update the logic in xfs_iread when the values are read from
disk to convert their on-disk values into the internal kernel
representation.

Update xfs_set_projid, xfs_set_uid, and xfs_set_gid to convert
from the internal kernel representation to the on-disk values
when updating the on-disk fields.

Update comparisons against these fields to use uid_eq, gid_eq
and projid_eq.

Cc: Ben Myers <bpm-sJ/iWh9BUns@public.gmane.org>
Cc: Alex Elder <elder-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org>
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
 fs/xfs/xfs_icache.c   |    6 +++---
 fs/xfs/xfs_inode.c    |   14 ++++++++------
 fs/xfs/xfs_inode.h    |   15 ++++++++-------
 fs/xfs/xfs_ioctl.c    |    8 ++++----
 fs/xfs/xfs_itable.c   |   10 ++++++----
 fs/xfs/xfs_qm.c       |   20 ++++++++++----------
 fs/xfs/xfs_rename.c   |    2 +-
 fs/xfs/xfs_vnodeops.c |    2 +-
 8 files changed, 41 insertions(+), 36 deletions(-)

diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index eba1dbd..0583649 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -1202,15 +1202,15 @@ xfs_inode_match_id(
 	struct xfs_eofblocks	*eofb)
 {
 	if (eofb->eof_flags & XFS_EOF_FLAGS_UID &&
-	    VFS_I(ip)->i_uid != eofb->eof_uid)
+	    !uid_eq(VFS_I(ip)->i_uid, eofb->eof_uid))
 		return 0;
 
 	if (eofb->eof_flags & XFS_EOF_FLAGS_GID &&
-	    VFS_I(ip)->i_gid != eofb->eof_gid)
+	    !gid_eq(VFS_I(ip)->i_gid, eofb->eof_gid))
 		return 0;
 
 	if (eofb->eof_flags & XFS_EOF_FLAGS_PRID &&
-	    ip->i_projid != eofb->eof_prid)
+	    !projid_eq(ip->i_projid, eofb->eof_prid))
 		return 0;
 
 	return 1;
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 846166d..af5420a 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1014,10 +1014,11 @@ xfs_iread(
 	if (dip->di_mode) {
 		xfs_dinode_from_disk(&ip->i_d, dip);
 
-		ip->i_projid = ((projid_t)ip->i_d.di_projid_hi << 16) |
-					  ip->i_d.di_projid_lo;
-		VFS_I(ip)->i_uid = ip->i_d.di_uid;
-		VFS_I(ip)->i_gid = ip->i_d.di_gid;
+		ip->i_projid = make_kprojid(&init_user_ns,
+					    ((projid_t)ip->i_d.di_projid_hi << 16) |
+					    ip->i_d.di_projid_lo);
+		VFS_I(ip)->i_uid = make_kuid(&init_user_ns, ip->i_d.di_uid);
+		VFS_I(ip)->i_gid = make_kgid(&init_user_ns, ip->i_d.di_gid);
 
 		error = xfs_iformat(ip, dip);
 		if (error)  {
@@ -1056,7 +1057,7 @@ xfs_iread(
 	if (ip->i_d.di_version == 1) {
 		ip->i_d.di_nlink = ip->i_d.di_onlink;
 		ip->i_d.di_onlink = 0;
-		xfs_set_projid(ip, 0);
+		xfs_set_projid(ip, make_kprojid(&init_user_ns, 0));
 	}
 
 	ip->i_delayed_blks = 0;
@@ -2845,7 +2846,8 @@ xfs_iflush_int(
 			memset(&(ip->i_d.di_pad[0]), 0, sizeof(ip->i_d.di_pad));
 			memset(&(dip->di_pad[0]), 0,
 			      sizeof(dip->di_pad));
-			ASSERT(ip->i_projid == 0);
+			ASSERT(projid_eq(ip->i_projid,
+					 make_kprojid(&init_user_ns, 0)));
 		}
 	}
 
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index f503aed..b82160f 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -244,7 +244,7 @@ typedef struct xfs_inode {
 	unsigned int		i_delayed_blks;	/* count of delay alloc blks */
 
 	xfs_icdinode_t		i_d;		/* most of ondisk inode */
-	projid_t		i_projid;	/* Project id */
+	kprojid_t		i_projid;	/* project id */
 
 	/* VFS inode */
 	struct inode		i_vnode;	/* embedded VFS inode */
@@ -363,23 +363,24 @@ xfs_iflags_test_and_set(xfs_inode_t *ip, unsigned short flags)
 
 static inline void
 xfs_set_projid(struct xfs_inode *ip,
-		prid_t projid)
+		kprojid_t kprojid)
 {
-	ip->i_projid = projid;
+	projid_t projid = from_kprojid(&init_user_ns, kprojid);
+	ip->i_projid = kprojid;
 	ip->i_d.di_projid_hi = (__uint16_t) (projid >> 16);
 	ip->i_d.di_projid_lo = (__uint16_t) (projid & 0xffff);
 }
 
-static inline void xfs_set_uid(struct xfs_inode *ip, uid_t uid)
+static inline void xfs_set_uid(struct xfs_inode *ip, kuid_t uid)
 {
 	VFS_I(ip)->i_uid = uid;
-	ip->i_d.di_uid = uid;
+	ip->i_d.di_uid = from_kuid(&init_user_ns, uid);
 }
 
-static inline void xfs_set_gid(struct xfs_inode *ip, gid_t gid)
+static inline void xfs_set_gid(struct xfs_inode *ip, kgid_t gid)
 {
 	VFS_I(ip)->i_gid = gid;
-	ip->i_d.di_gid = gid;
+	ip->i_d.di_gid = from_kgid(&init_user_ns, gid);
 }
 
 /*
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index db274d4..016624b 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -815,7 +815,7 @@ xfs_ioc_fsgetxattr(
 	xfs_ilock(ip, XFS_ILOCK_SHARED);
 	fa.fsx_xflags = xfs_ip2xflags(ip);
 	fa.fsx_extsize = ip->i_d.di_extsize << ip->i_mount->m_sb.sb_blocklog;
-	fa.fsx_projid = ip->i_projid;
+	fa.fsx_projid = from_kprojid_munged(current_user_ns(), ip->i_projid);
 
 	if (attr) {
 		if (ip->i_afp) {
@@ -975,7 +975,7 @@ xfs_ioctl_setattr(
 	 * to the file owner ID, except in cases where the
 	 * CAP_FSETID capability is applicable.
 	 */
-	if (current_fsuid() != VFS_I(ip)->i_uid && !capable(CAP_FOWNER)) {
+	if (!uid_eq(current_fsuid(), VFS_I(ip)->i_uid) && !capable(CAP_FOWNER)) {
 		code = XFS_ERROR(EPERM);
 		goto error_return;
 	}
@@ -986,7 +986,7 @@ xfs_ioctl_setattr(
 	if (mask & FSX_PROJID) {
 		if (XFS_IS_QUOTA_RUNNING(mp) &&
 		    XFS_IS_PQUOTA_ON(mp) &&
-		    ip->i_projid != fa->fsx_projid) {
+		    !projid_eq(ip->i_projid, fa->fsx_projid)) {
 			ASSERT(tp);
 			code = xfs_qm_vop_chown_reserve(tp, ip, udqp, gdqp,
 						capable(CAP_FOWNER) ?
@@ -1104,7 +1104,7 @@ xfs_ioctl_setattr(
 		 * Change the ownerships and register quota modifications
 		 * in the transaction.
 		 */
-		if (ip->i_projid != fa->fsx_projid) {
+		if (!projid_eq(ip->i_projid, fa->fsx_projid)) {
 			if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_PQUOTA_ON(mp)) {
 				olddquot = xfs_qm_vop_chown(tp, ip,
 							&ip->i_gdquot, gdqp);
diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index a9e07dd..f639773 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -64,6 +64,7 @@ xfs_bulkstat_one_int(
 	struct xfs_inode	*ip;		/* incore inode pointer */
 	struct xfs_bstat	*buf;		/* return buffer */
 	int			error = 0;	/* error value */
+	projid_t		projid;
 
 	*stat = BULKSTAT_RV_NOTHING;
 
@@ -91,12 +92,13 @@ xfs_bulkstat_one_int(
 	 * further change.
 	 */
 	buf->bs_nlink = dic->di_nlink;
-	buf->bs_projid_lo = (u16)(ip->i_projid & 0xffff);
-	buf->bs_projid_hi = (u16)(ip->i_projid >> 16);
+	projid = from_kprojid_munged(current_user_ns(), ip->i_projid);
+	buf->bs_projid_lo = (u16)(projid & 0xffff);
+	buf->bs_projid_hi = (u16)(projid >> 16);
 	buf->bs_ino = ino;
 	buf->bs_mode = dic->di_mode;
-	buf->bs_uid = VFS_I(ip)->i_uid;
-	buf->bs_gid = VFS_I(ip)->i_gid;
+	buf->bs_uid = from_kuid_munged(current_user_ns(), VFS_I(ip)->i_uid);
+	buf->bs_gid = from_kgid_munged(current_user_ns(), VFS_I(ip)->i_gid);
 	buf->bs_size = dic->di_size;
 	buf->bs_atime.tv_sec = dic->di_atime.t_sec;
 	buf->bs_atime.tv_nsec = dic->di_atime.t_nsec;
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index c1310ed..f11e10d 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -1650,7 +1650,7 @@ xfs_qm_vop_dqalloc(
 
 	uq = gq = NULL;
 	if ((flags & XFS_QMOPT_UQUOTA) && XFS_IS_UQUOTA_ON(mp)) {
-		if (VFS_I(ip)->i_uid != uid) {
+		if (!uid_eq(VFS_I(ip)->i_uid, uid)) {
 			/*
 			 * What we need is the dquot that has this uid, and
 			 * if we send the inode to dqget, the uid of the inode
@@ -1685,7 +1685,7 @@ xfs_qm_vop_dqalloc(
 		}
 	}
 	if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
-		if (VFS_I(ip)->i_gid != gid) {
+		if (!gid_eq(VFS_I(ip)->i_gid, gid)) {
 			xfs_iunlock(ip, lockflags);
 			if ((error = xfs_qm_dqget(mp, NULL, (xfs_dqid_t)gid,
 						 XFS_DQ_GROUP,
@@ -1705,7 +1705,7 @@ xfs_qm_vop_dqalloc(
 			gq = xfs_qm_dqhold(ip->i_gdquot);
 		}
 	} else if ((flags & XFS_QMOPT_PQUOTA) && XFS_IS_PQUOTA_ON(mp)) {
-		if (ip->i_projid != prid) {
+		if (!projid_eq(ip->i_projid, prid)) {
 			xfs_iunlock(ip, lockflags);
 			if ((error = xfs_qm_dqget(mp, NULL, (xfs_dqid_t)prid,
 						 XFS_DQ_PROJ,
@@ -1806,7 +1806,7 @@ xfs_qm_vop_chown_reserve(
 			XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS;
 
 	if (XFS_IS_UQUOTA_ON(mp) && udqp &&
-	    VFS_I(ip)->i_uid != (uid_t)be32_to_cpu(udqp->q_core.d_id)) {
+	    !uid_eq(VFS_I(ip)->i_uid, (uid_t)be32_to_cpu(udqp->q_core.d_id))) {
 		delblksudq = udqp;
 		/*
 		 * If there are delayed allocation blocks, then we have to
@@ -1820,12 +1820,12 @@ xfs_qm_vop_chown_reserve(
 	}
 	if (XFS_IS_OQUOTA_ON(ip->i_mount) && gdqp) {
 		if (XFS_IS_PQUOTA_ON(ip->i_mount) &&
-		     ip->i_projid != be32_to_cpu(gdqp->q_core.d_id))
+		    !projid_eq(ip->i_projid, be32_to_cpu(gdqp->q_core.d_id)))
 			prjflags = XFS_QMOPT_ENOSPC;
 
 		if (prjflags ||
 		    (XFS_IS_GQUOTA_ON(ip->i_mount) &&
-		     VFS_I(ip)->i_gid != be32_to_cpu(gdqp->q_core.d_id))) {
+		     !gid_eq(VFS_I(ip)->i_gid, be32_to_cpu(gdqp->q_core.d_id)))) {
 			delblksgdq = gdqp;
 			if (delblks) {
 				ASSERT(ip->i_gdquot);
@@ -1909,7 +1909,7 @@ xfs_qm_vop_create_dqattach(
 	if (udqp) {
 		ASSERT(ip->i_udquot == NULL);
 		ASSERT(XFS_IS_UQUOTA_ON(mp));
-		ASSERT(VFS_I(ip)->i_uid == be32_to_cpu(udqp->q_core.d_id));
+		ASSERT(uid_eq(VFS_I(ip)->i_uid, be32_to_cpu(udqp->q_core.d_id)));
 
 		ip->i_udquot = xfs_qm_dqhold(udqp);
 		xfs_trans_mod_dquot(tp, udqp, XFS_TRANS_DQ_ICOUNT, 1);
@@ -1917,9 +1917,9 @@ xfs_qm_vop_create_dqattach(
 	if (gdqp) {
 		ASSERT(ip->i_gdquot == NULL);
 		ASSERT(XFS_IS_OQUOTA_ON(mp));
-		ASSERT((XFS_IS_GQUOTA_ON(mp) ?
-			VFS_I(ip)->i_gid : ip->i_projid) ==
-				be32_to_cpu(gdqp->q_core.d_id));
+		ASSERT(XFS_IS_GQUOTA_ON(mp) ?
+		       gid_eq(VFS_I(ip)->i_gid, be32_to_cpu(gdqp->q_core.d_id)):
+		       projid_eq(ip->i_projid, be32_to_cpu(gdqp->q_core.d_id)));
 
 		ip->i_gdquot = xfs_qm_dqhold(gdqp);
 		xfs_trans_mod_dquot(tp, gdqp, XFS_TRANS_DQ_ICOUNT, 1);
diff --git a/fs/xfs/xfs_rename.c b/fs/xfs/xfs_rename.c
index 89981bb..9cc22b2 100644
--- a/fs/xfs/xfs_rename.c
+++ b/fs/xfs/xfs_rename.c
@@ -171,7 +171,7 @@ xfs_rename(
 	 * tree quota mechanism would be circumvented.
 	 */
 	if (unlikely((target_dp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT) &&
-		     (target_dp->i_projid != src_ip->i_projid))) {
+		     !projid_eq(target_dp->i_projid, src_ip->i_projid))) {
 		error = XFS_ERROR(EXDEV);
 		goto error_return;
 	}
diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
index 7a2c2a0..e7b27c8 100644
--- a/fs/xfs/xfs_vnodeops.c
+++ b/fs/xfs/xfs_vnodeops.c
@@ -1305,7 +1305,7 @@ xfs_link(
 	 * the tree quota mechanism could be circumvented.
 	 */
 	if (unlikely((tdp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT) &&
-		     (tdp->i_projid != sip->i_projid))) {
+		     !projid_eq(tdp->i_projid, sip->i_projid))) {
 		error = XFS_ERROR(EXDEV);
 		goto error_return;
 	}
-- 
1.7.5.4

[PATCH review 05/16] xfs: Update xfs_ioctl_setattr to handle projids in any user namespace

[Eric W. Biederman]

From: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>

- Convert the userspace value in fa->fsx_projid into a kprojid and
  store it in the variable projid.
- Verify that xfs can store the projid after it is converted into
  xfs's user namespace.
- Replace uses of fa->fsx_projid with projid throughout
  xfs_ioctl_setattr.

Cc: Ben Myers <bpm-sJ/iWh9BUns@public.gmane.org>
Cc: Alex Elder <elder-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org>
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
 fs/xfs/xfs_ioctl.c |   26 ++++++++++++++++++--------
 1 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 016624b..4a55f50 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -925,6 +925,7 @@ xfs_ioctl_setattr(
 	struct xfs_dquot	*gdqp = NULL;
 	struct xfs_dquot	*olddquot = NULL;
 	int			code;
+	kprojid_t		projid = INVALID_PROJID;
 
 	trace_xfs_ioctl_setattr(ip);
 
@@ -934,11 +935,20 @@ xfs_ioctl_setattr(
 		return XFS_ERROR(EIO);
 
 	/*
-	 * Disallow 32bit project ids when projid32bit feature is not enabled.
+	 * Verify the specifid project id is valid.
 	 */
-	if ((mask & FSX_PROJID) && (fa->fsx_projid > (__uint16_t)-1) &&
-			!xfs_sb_version_hasprojid32bit(&ip->i_mount->m_sb))
-		return XFS_ERROR(EINVAL);
+	if (mask & FSX_PROJID) {
+		projid = make_kprojid(current_user_ns(), fa->fsx_projid);
+		if (!projid_valid(projid))
+			return XFS_ERROR(EINVAL);
+
+		/*
+		 * Disallow 32bit project ids when projid32bit feature is not enabled.
+		 */
+		if ((from_kprojid(&init_user_ns, projid) > (__uint16_t)-1) &&
+		    !xfs_sb_version_hasprojid32bit(&ip->i_mount->m_sb))
+			return XFS_ERROR(EINVAL);
+	}
 
 	/*
 	 * If disk quotas is on, we make sure that the dquots do exist on disk,
@@ -950,7 +960,7 @@ xfs_ioctl_setattr(
 	 */
 	if (XFS_IS_QUOTA_ON(mp) && (mask & FSX_PROJID)) {
 		code = xfs_qm_vop_dqalloc(ip, VFS_I(ip)->i_uid,
-					 VFS_I(ip)->i_gid, fa->fsx_projid,
+					 VFS_I(ip)->i_gid, projid,
 					 XFS_QMOPT_PQUOTA, &udqp, &gdqp);
 		if (code)
 			return code;
@@ -986,7 +996,7 @@ xfs_ioctl_setattr(
 	if (mask & FSX_PROJID) {
 		if (XFS_IS_QUOTA_RUNNING(mp) &&
 		    XFS_IS_PQUOTA_ON(mp) &&
-		    !projid_eq(ip->i_projid, fa->fsx_projid)) {
+		    !projid_eq(ip->i_projid, projid)) {
 			ASSERT(tp);
 			code = xfs_qm_vop_chown_reserve(tp, ip, udqp, gdqp,
 						capable(CAP_FOWNER) ?
@@ -1104,12 +1114,12 @@ xfs_ioctl_setattr(
 		 * Change the ownerships and register quota modifications
 		 * in the transaction.
 		 */
-		if (!projid_eq(ip->i_projid, fa->fsx_projid)) {
+		if (!projid_eq(ip->i_projid, projid)) {
 			if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_PQUOTA_ON(mp)) {
 				olddquot = xfs_qm_vop_chown(tp, ip,
 							&ip->i_gdquot, gdqp);
 			}
-			xfs_set_projid(ip, fa->fsx_projid);
+			xfs_set_projid(ip, projid);
 
 			/*
 			 * We may have to rev the inode as well as
-- 
1.7.5.4

Re: [PATCH review 05/16] xfs: Update xfs_ioctl_setattr to handle projids in any user namespace

[Dave Chinner]

On Sun, Feb 17, 2013 at 05:10:58PM -0800, Eric W. Biederman wrote:
> From: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
> 
> - Convert the userspace value in fa->fsx_projid into a kprojid and
>   store it in the variable projid.
> - Verify that xfs can store the projid after it is converted into
>   xfs's user namespace.
> - Replace uses of fa->fsx_projid with projid throughout
>   xfs_ioctl_setattr.
> 
> Cc: Ben Myers <bpm-sJ/iWh9BUns@public.gmane.org>
> Cc: Alex Elder <elder-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org>
> Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
> ---
>  fs/xfs/xfs_ioctl.c |   26 ++++++++++++++++++--------
>  1 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 016624b..4a55f50 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -925,6 +925,7 @@ xfs_ioctl_setattr(
>  	struct xfs_dquot	*gdqp = NULL;
>  	struct xfs_dquot	*olddquot = NULL;
>  	int			code;
> +	kprojid_t		projid = INVALID_PROJID;
>  
>  	trace_xfs_ioctl_setattr(ip);
>  
> @@ -934,11 +935,20 @@ xfs_ioctl_setattr(
>  		return XFS_ERROR(EIO);
>  
>  	/*
> -	 * Disallow 32bit project ids when projid32bit feature is not enabled.
> +	 * Verify the specifid project id is valid.
>  	 */
> -	if ((mask & FSX_PROJID) && (fa->fsx_projid > (__uint16_t)-1) &&
> -			!xfs_sb_version_hasprojid32bit(&ip->i_mount->m_sb))
> -		return XFS_ERROR(EINVAL);
> +	if (mask & FSX_PROJID) {
> +		projid = make_kprojid(current_user_ns(), fa->fsx_projid);
> +		if (!projid_valid(projid))
> +			return XFS_ERROR(EINVAL);
> +
> +		/*
> +		 * Disallow 32bit project ids when projid32bit feature is not enabled.
> +		 */
> +		if ((from_kprojid(&init_user_ns, projid) > (__uint16_t)-1) &&
> +		    !xfs_sb_version_hasprojid32bit(&ip->i_mount->m_sb))
> +			return XFS_ERROR(EINVAL);
> +	}

That looks busted. Why does one use current_user_ns() and the other
&init_user_ns()?

Cheers,

Dave.
-- 
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org

Re: [PATCH review 05/16] xfs: Update xfs_ioctl_setattr to handle projids in any user namespace

[Gao feng]

On 02/19/2013 09:55 AM, Dave Chinner wrote:
> On Sun, Feb 17, 2013 at 05:10:58PM -0800, Eric W. Biederman wrote:
>> From: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
>>
>> - Convert the userspace value in fa->fsx_projid into a kprojid and
>>   store it in the variable projid.
>> - Verify that xfs can store the projid after it is converted into
>>   xfs's user namespace.
>> - Replace uses of fa->fsx_projid with projid throughout
>>   xfs_ioctl_setattr.
>>
>> Cc: Ben Myers <bpm-sJ/iWh9BUns@public.gmane.org>
>> Cc: Alex Elder <elder-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Cc: Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org>
>> Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
>> ---
>>  fs/xfs/xfs_ioctl.c |   26 ++++++++++++++++++--------
>>  1 files changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
>> index 016624b..4a55f50 100644
>> --- a/fs/xfs/xfs_ioctl.c
>> +++ b/fs/xfs/xfs_ioctl.c
>> @@ -925,6 +925,7 @@ xfs_ioctl_setattr(
>>  	struct xfs_dquot	*gdqp = NULL;
>>  	struct xfs_dquot	*olddquot = NULL;
>>  	int			code;
>> +	kprojid_t		projid = INVALID_PROJID;
>>  
>>  	trace_xfs_ioctl_setattr(ip);
>>  
>> @@ -934,11 +935,20 @@ xfs_ioctl_setattr(
>>  		return XFS_ERROR(EIO);
>>  
>>  	/*
>> -	 * Disallow 32bit project ids when projid32bit feature is not enabled.
>> +	 * Verify the specifid project id is valid.
>>  	 */
>> -	if ((mask & FSX_PROJID) && (fa->fsx_projid > (__uint16_t)-1) &&
>> -			!xfs_sb_version_hasprojid32bit(&ip->i_mount->m_sb))
>> -		return XFS_ERROR(EINVAL);
>> +	if (mask & FSX_PROJID) {
>> +		projid = make_kprojid(current_user_ns(), fa->fsx_projid);
>> +		if (!projid_valid(projid))
>> +			return XFS_ERROR(EINVAL);
>> +
>> +		/*
>> +		 * Disallow 32bit project ids when projid32bit feature is not enabled.
>> +		 */
>> +		if ((from_kprojid(&init_user_ns, projid) > (__uint16_t)-1) &&
>> +		    !xfs_sb_version_hasprojid32bit(&ip->i_mount->m_sb))
>> +			return XFS_ERROR(EINVAL);
>> +	}
> 
> That looks busted. Why does one use current_user_ns() and the other
> &init_user_ns()?
> 

hmm, through this thread had been stopped discussing for a long time, but I'm working on converting
ids to kids for xfs now, and I want to remove the finial dependenciy for user namespace.

Maybe I can explain why.

projid = make_kprojid(current_user_ns(), fa->fsx_projid);

this code translates the projid in current user namespace to the global kernel projid(kprojid_t).

and from_kprojid(&init_user_ns, projid) here is equal to projid.val, since the real value
of global kernel projid and projid is same in init user namespace.

After translating projid to global kernel projid, we should check if the real value of this
global kernel projid is valid.

I think maybe just modifying from_kprojid(&init_user_ns, projid) to projid.val here is easier
to understand.


Thanks!

Re: [PATCH review 05/16] xfs: Update xfs_ioctl_setattr to handle projids in any user namespace

[Dave Chinner]

[ cc xfs list ]

On Mon, Jul 29, 2013 at 03:17:06PM +0800, Gao feng wrote:
> On 02/19/2013 09:55 AM, Dave Chinner wrote:
> > On Sun, Feb 17, 2013 at 05:10:58PM -0800, Eric W. Biederman wrote:
> >> From: "Eric W. Biederman" <ebiederm@xmission.com>
> >>
> >> - Convert the userspace value in fa->fsx_projid into a kprojid and
> >>   store it in the variable projid.
> >> - Verify that xfs can store the projid after it is converted into
> >>   xfs's user namespace.
> >> - Replace uses of fa->fsx_projid with projid throughout
> >>   xfs_ioctl_setattr.
> >>
> >> Cc: Ben Myers <bpm@sgi.com>
> >> Cc: Alex Elder <elder@kernel.org>
> >> Cc: Dave Chinner <david@fromorbit.com>
> >> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> >> ---
> >>  fs/xfs/xfs_ioctl.c |   26 ++++++++++++++++++--------
> >>  1 files changed, 18 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> >> index 016624b..4a55f50 100644
> >> --- a/fs/xfs/xfs_ioctl.c
> >> +++ b/fs/xfs/xfs_ioctl.c
> >> @@ -925,6 +925,7 @@ xfs_ioctl_setattr(
> >>  	struct xfs_dquot	*gdqp = NULL;
> >>  	struct xfs_dquot	*olddquot = NULL;
> >>  	int			code;
> >> +	kprojid_t		projid = INVALID_PROJID;
> >>  
> >>  	trace_xfs_ioctl_setattr(ip);
> >>  
> >> @@ -934,11 +935,20 @@ xfs_ioctl_setattr(
> >>  		return XFS_ERROR(EIO);
> >>  
> >>  	/*
> >> -	 * Disallow 32bit project ids when projid32bit feature is not enabled.
> >> +	 * Verify the specifid project id is valid.
> >>  	 */
> >> -	if ((mask & FSX_PROJID) && (fa->fsx_projid > (__uint16_t)-1) &&
> >> -			!xfs_sb_version_hasprojid32bit(&ip->i_mount->m_sb))
> >> -		return XFS_ERROR(EINVAL);
> >> +	if (mask & FSX_PROJID) {
> >> +		projid = make_kprojid(current_user_ns(), fa->fsx_projid);
> >> +		if (!projid_valid(projid))
> >> +			return XFS_ERROR(EINVAL);
> >> +
> >> +		/*
> >> +		 * Disallow 32bit project ids when projid32bit feature is not enabled.
> >> +		 */
> >> +		if ((from_kprojid(&init_user_ns, projid) > (__uint16_t)-1) &&
> >> +		    !xfs_sb_version_hasprojid32bit(&ip->i_mount->m_sb))
> >> +			return XFS_ERROR(EINVAL);
> >> +	}
> > 
> > That looks busted. Why does one use current_user_ns() and the other
> > &init_user_ns()?
> > 
> 
> hmm, through this thread had been stopped discussing for a long time, but I'm working on converting
> ids to kids for xfs now, and I want to remove the finial dependenciy for user namespace.

You're duplicating work that is already going on - we've been
talking about this stuff on the XFS list and reviewing patches for
the last 3-4 weeks for this.

http://oss.sgi.com/pipermail/xfs/2013-July/028467.html

Basically, the discussion we are currently having is whether project
IDs should be exposed to user namespaces at all. e.g:

http://oss.sgi.com/pipermail/xfs/2013-July/028497.html
http://oss.sgi.com/pipermail/xfs/2013-July/028551.html

"Basically, until we have worked out *if* project quotas can be used
safely within user namespaces, we need to reject any attempt to use
them from within a user namespace container."

i.e. the whole "project IDs are part of user namespaces because of
quotas" looks like a bad decision to have been made. Project IDs are
independent of users and groups and can be used to account for usage
across discontiguous directory and permission heirarchies, so trying
to contain them to a "user namespace" really matches their
functionality.

> I think maybe just modifying from_kprojid(&init_user_ns, projid) to projid.val here is easier
> to understand.

The problem is not a question of how to implement mappings - it's a
question of how to deal with fundamental impedence mismatch of
user namespaces and project IDs....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH review 05/16] xfs: Update xfs_ioctl_setattr to handle projids in any user namespace

[Gao feng]

On 07/29/2013 03:51 PM, Dave Chinner wrote:
> [ cc xfs list ]
> 
> On Mon, Jul 29, 2013 at 03:17:06PM +0800, Gao feng wrote:
>> On 02/19/2013 09:55 AM, Dave Chinner wrote:
>>> On Sun, Feb 17, 2013 at 05:10:58PM -0800, Eric W. Biederman wrote:
>>>> From: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
>>>>
>>>> - Convert the userspace value in fa->fsx_projid into a kprojid and
>>>>   store it in the variable projid.
>>>> - Verify that xfs can store the projid after it is converted into
>>>>   xfs's user namespace.
>>>> - Replace uses of fa->fsx_projid with projid throughout
>>>>   xfs_ioctl_setattr.
>>>>
>>>> Cc: Ben Myers <bpm-sJ/iWh9BUns@public.gmane.org>
>>>> Cc: Alex Elder <elder-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>>> Cc: Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org>
>>>> Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
>>>> ---
>>>>  fs/xfs/xfs_ioctl.c |   26 ++++++++++++++++++--------
>>>>  1 files changed, 18 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
>>>> index 016624b..4a55f50 100644
>>>> --- a/fs/xfs/xfs_ioctl.c
>>>> +++ b/fs/xfs/xfs_ioctl.c
>>>> @@ -925,6 +925,7 @@ xfs_ioctl_setattr(
>>>>  	struct xfs_dquot	*gdqp = NULL;
>>>>  	struct xfs_dquot	*olddquot = NULL;
>>>>  	int			code;
>>>> +	kprojid_t		projid = INVALID_PROJID;
>>>>  
>>>>  	trace_xfs_ioctl_setattr(ip);
>>>>  
>>>> @@ -934,11 +935,20 @@ xfs_ioctl_setattr(
>>>>  		return XFS_ERROR(EIO);
>>>>  
>>>>  	/*
>>>> -	 * Disallow 32bit project ids when projid32bit feature is not enabled.
>>>> +	 * Verify the specifid project id is valid.
>>>>  	 */
>>>> -	if ((mask & FSX_PROJID) && (fa->fsx_projid > (__uint16_t)-1) &&
>>>> -			!xfs_sb_version_hasprojid32bit(&ip->i_mount->m_sb))
>>>> -		return XFS_ERROR(EINVAL);
>>>> +	if (mask & FSX_PROJID) {
>>>> +		projid = make_kprojid(current_user_ns(), fa->fsx_projid);
>>>> +		if (!projid_valid(projid))
>>>> +			return XFS_ERROR(EINVAL);
>>>> +
>>>> +		/*
>>>> +		 * Disallow 32bit project ids when projid32bit feature is not enabled.
>>>> +		 */
>>>> +		if ((from_kprojid(&init_user_ns, projid) > (__uint16_t)-1) &&
>>>> +		    !xfs_sb_version_hasprojid32bit(&ip->i_mount->m_sb))
>>>> +			return XFS_ERROR(EINVAL);
>>>> +	}
>>>
>>> That looks busted. Why does one use current_user_ns() and the other
>>> &init_user_ns()?
>>>
>>
>> hmm, through this thread had been stopped discussing for a long time, but I'm working on converting
>> ids to kids for xfs now, and I want to remove the finial dependenciy for user namespace.
> 
> You're duplicating work that is already going on - we've been
> talking about this stuff on the XFS list and reviewing patches for
> the last 3-4 weeks for this.
> 

oops.. thanks for your reminder!

> http://oss.sgi.com/pipermail/xfs/2013-July/028467.html
> 
> Basically, the discussion we are currently having is whether project
> IDs should be exposed to user namespaces at all. e.g:
> 
> http://oss.sgi.com/pipermail/xfs/2013-July/028497.html
> http://oss.sgi.com/pipermail/xfs/2013-July/028551.html
> 
> "Basically, until we have worked out *if* project quotas can be used
> safely within user namespaces, we need to reject any attempt to use
> them from within a user namespace container."
> 

yes, seems this v6 patchset allows user in un-init user namespace to setup proj quota
through ioctl, and the projid hasn't been converted to kprojid in this patchset.
Doesn't this will cause user in container has the ability to change the proj quota
which is set by root user in host?

> i.e. the whole "project IDs are part of user namespaces because of
> quotas" looks like a bad decision to have been made. Project IDs are
> independent of users and groups and can be used to account for usage
> across discontiguous directory and permission heirarchies, so trying
> to contain them to a "user namespace" really matches their
> functionality.

I'm ok if we disallow un-init user namespace to setup proj quota right now,
all user namespace will share the same proj quota and only init user namespace
has the rights to modify proj quota. this looks clear to me.


Thanks,
Gao

Re: [PATCH review 05/16] xfs: Update xfs_ioctl_setattr to handle projids in any user namespace

[Dave Chinner]

On Tue, Jul 30, 2013 at 11:15:50AM +0800, Gao feng wrote:
> On 07/29/2013 03:51 PM, Dave Chinner wrote:
> > http://oss.sgi.com/pipermail/xfs/2013-July/028467.html
> > 
> > Basically, the discussion we are currently having is whether project
> > IDs should be exposed to user namespaces at all. e.g:
> > 
> > http://oss.sgi.com/pipermail/xfs/2013-July/028497.html
> > http://oss.sgi.com/pipermail/xfs/2013-July/028551.html
> > 
> > "Basically, until we have worked out *if* project quotas can be used
> > safely within user namespaces, we need to reject any attempt to use
> > them from within a user namespace container."
> > 
> 
> yes, seems this v6 patchset allows user in un-init user namespace to setup proj quota
> through ioctl, and the projid hasn't been converted to kprojid in this patchset.
> Doesn't this will cause user in container has the ability to change the proj quota
> which is set by root user in host?

Dwight just posted v7. can you discuss your concerns in reposnse to
the relevant patch in that series, please? it's much easier for
everyone if we keep the discussion int eh one thread ;)

Cheers,

Dave.
-- 
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org

Re: [PATCH review 05/16] xfs: Update xfs_ioctl_setattr to handle projids in any user namespace

[Gao feng]

On 07/30/2013 11:57 AM, Dave Chinner wrote:
> On Tue, Jul 30, 2013 at 11:15:50AM +0800, Gao feng wrote:
>> On 07/29/2013 03:51 PM, Dave Chinner wrote:
>>> http://oss.sgi.com/pipermail/xfs/2013-July/028467.html
>>>
>>> Basically, the discussion we are currently having is whether project
>>> IDs should be exposed to user namespaces at all. e.g:
>>>
>>> http://oss.sgi.com/pipermail/xfs/2013-July/028497.html
>>> http://oss.sgi.com/pipermail/xfs/2013-July/028551.html
>>>
>>> "Basically, until we have worked out *if* project quotas can be used
>>> safely within user namespaces, we need to reject any attempt to use
>>> them from within a user namespace container."
>>>
>>
>> yes, seems this v6 patchset allows user in un-init user namespace to setup proj quota
>> through ioctl, and the projid hasn't been converted to kprojid in this patchset.
>> Doesn't this will cause user in container has the ability to change the proj quota
>> which is set by root user in host?
> 
> Dwight just posted v7. can you discuss your concerns in reposnse to
> the relevant patch in that series, please? it's much easier for
> everyone if we keep the discussion int eh one thread ;)
> 

sure, I am compiling  v7 patchset now in order to confirm my misgiving :)

Thanks

[PATCH review 06/16] xfs: Use kuids and kgids in xfs_setattr_nonsize

[Eric W. Biederman]

From: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>

Cc: Ben Myers <bpm-sJ/iWh9BUns@public.gmane.org>
Cc: Alex Elder <elder-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org>
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
 fs/xfs/xfs_iops.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 235a48c..0edacbe 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -466,8 +466,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;
 
@@ -549,8 +549,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) ?
@@ -580,7 +580,7 @@ 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);
@@ -589,7 +589,7 @@ xfs_setattr_nonsize(
 			}
 			xfs_set_uid(ip, 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);
-- 
1.7.5.4

[PATCH review 07/16] xfs: Update ioctl(XFS_IOC_FREE_EOFBLOCKS) to handle callers in any userspace

[Eric W. Biederman]

From: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>

- Modify the ioctl to convert from uids, gid, and projids in the
  current user namespace to kuids, kgids, and kprojids, and to report
  an error if the conversion fails.

- Create struct xfs_internal_eofblocks to hold the same information as
  struct xfs_eofblocks but with uids, gids, and projids stored as
  kuids, kgids, and kprojids preventing confusion.

- Pass struct xfs_interanl_eofblocks into xfs_icache_free_eofblocks
  and it's helpers ensuring there will not be confusing about which
  user namespace identifiers that need to be compared are in.

Cc: Ben Myers <bpm-sJ/iWh9BUns@public.gmane.org>
Cc: Alex Elder <elder-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org>
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
 fs/xfs/xfs_icache.c |    8 ++++----
 fs/xfs/xfs_icache.h |   11 ++++++++++-
 fs/xfs/xfs_ioctl.c  |   22 +++++++++++++++++++++-
 3 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 0583649..e4cdc02 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -1199,7 +1199,7 @@ xfs_reclaim_inodes_count(
 STATIC int
 xfs_inode_match_id(
 	struct xfs_inode	*ip,
-	struct xfs_eofblocks	*eofb)
+	struct xfs_internal_eofblocks	*eofb)
 {
 	if (eofb->eof_flags & XFS_EOF_FLAGS_UID &&
 	    !uid_eq(VFS_I(ip)->i_uid, eofb->eof_uid))
@@ -1210,7 +1210,7 @@ xfs_inode_match_id(
 		return 0;
 
 	if (eofb->eof_flags & XFS_EOF_FLAGS_PRID &&
-	    !projid_eq(ip->i_projid, eofb->eof_prid))
+	    !projid_eq(ip->i_projid, eofb->eof_projid))
 		return 0;
 
 	return 1;
@@ -1224,7 +1224,7 @@ xfs_inode_free_eofblocks(
 	void			*args)
 {
 	int ret;
-	struct xfs_eofblocks *eofb = args;
+	struct xfs_internal_eofblocks *eofb = args;
 
 	if (!xfs_can_free_eofblocks(ip, false)) {
 		/* inode could be preallocated or append-only */
@@ -1263,7 +1263,7 @@ xfs_inode_free_eofblocks(
 int
 xfs_icache_free_eofblocks(
 	struct xfs_mount	*mp,
-	struct xfs_eofblocks	*eofb)
+	struct xfs_internal_eofblocks	*eofb)
 {
 	int flags = SYNC_TRYLOCK;
 
diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
index e0f138c..260dc27 100644
--- a/fs/xfs/xfs_icache.h
+++ b/fs/xfs/xfs_icache.h
@@ -35,9 +35,18 @@ void xfs_reclaim_inodes_nr(struct xfs_mount *mp, int nr_to_scan);
 
 void xfs_inode_set_reclaim_tag(struct xfs_inode *ip);
 
+struct xfs_internal_eofblocks {
+	u32		eof_version;
+	u32		eof_flags;
+	kuid_t		eof_uid;
+	kgid_t		eof_gid;
+	kprojid_t	eof_projid;
+	u64		eof_min_file_size;
+};
+
 void xfs_inode_set_eofblocks_tag(struct xfs_inode *ip);
 void xfs_inode_clear_eofblocks_tag(struct xfs_inode *ip);
-int xfs_icache_free_eofblocks(struct xfs_mount *, struct xfs_eofblocks *);
+int xfs_icache_free_eofblocks(struct xfs_mount *, struct xfs_internal_eofblocks *);
 void xfs_eofblocks_worker(struct work_struct *);
 
 int xfs_sync_inode_grab(struct xfs_inode *ip);
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 4a55f50..1a74b56 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1615,6 +1615,7 @@ xfs_file_ioctl(
 
 	case XFS_IOC_FREE_EOFBLOCKS: {
 		struct xfs_eofblocks eofb;
+		struct xfs_internal_eofblocks keofb;
 
 		if (copy_from_user(&eofb, arg, sizeof(eofb)))
 			return -XFS_ERROR(EFAULT);
@@ -1629,7 +1630,26 @@ xfs_file_ioctl(
 		    memchr_inv(eofb.pad64, 0, sizeof(eofb.pad64)))
 			return -XFS_ERROR(EINVAL);
 
-		error = xfs_icache_free_eofblocks(mp, &eofb);
+		keofb.eof_version = eofb.eof_version;
+		keofb.eof_flags   = eofb.eof_flags;
+		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 (eofb.eof_flags & XFS_EOF_FLAGS_PRID) {
+			keofb.eof_projid = make_kprojid(current_user_ns(), eofb.eof_prid);
+			if (!projid_valid(keofb.eof_projid))
+				return -XFS_ERROR(EINVAL);
+		}
+		keofb.eof_min_file_size = eofb.eof_min_file_size;
+
+		error = xfs_icache_free_eofblocks(mp, &keofb);
 		return -error;
 	}
 
-- 
1.7.5.4

Re: [PATCH review 07/16] xfs: Update ioctl(XFS_IOC_FREE_EOFBLOCKS) to handle callers in any userspace

[Dave Chinner]

On Sun, Feb 17, 2013 at 05:11:00PM -0800, Eric W. Biederman wrote:
> From: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
> 
> - Modify the ioctl to convert from uids, gid, and projids in the
>   current user namespace to kuids, kgids, and kprojids, and to report
>   an error if the conversion fails.

Please just convert to xfs_dqid_t at the interface.

> - Create struct xfs_internal_eofblocks to hold the same information as
>   struct xfs_eofblocks but with uids, gids, and projids stored as
>   kuids, kgids, and kprojids preventing confusion.

No need. Just convert the struct xfs_eof_blocks to define them all
as xfs_dqid_t and convert them in place to the type that is
compatible with the XFS core use of these fields (i.e. comparing
them with the on-disk inode uid/gid/prid values).

Cheers,

Dave.
-- 
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org

[PATCH review 11/16] xfs: Modify xfs_qm_dqget to take a struct kqid.

[Eric W. Biederman]

From: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>

- Modify xfs_qm_dqget, xfs_qm_dqattach_one, and xfs_qm_qutoacheck_dqadjust
  to take a struct kqid instead of an id and type pair.

- Modify their callers to pass them a struct kqid.

- Move xfs_qutoa_type into xfs_dquot.c where it is now used.

Cc: Ben Myers <bpm-sJ/iWh9BUns@public.gmane.org>
Cc: Alex Elder <elder-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org>
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
 fs/xfs/xfs_dquot.c       |   24 +++++++++++++++++++-----
 fs/xfs/xfs_dquot.h       |    4 ++--
 fs/xfs/xfs_qm.c          |   42 +++++++++++++++++++-----------------------
 fs/xfs/xfs_qm_bhv.c      |    2 +-
 fs/xfs/xfs_qm_syscalls.c |   20 ++------------------
 5 files changed, 43 insertions(+), 49 deletions(-)

diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 9e1bf52..6be5a29 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -63,6 +63,19 @@ static struct kmem_zone		*xfs_qm_dqzone;
 
 static struct lock_class_key xfs_dquot_other_class;
 
+STATIC int
+xfs_quota_type(int type)
+{
+	switch (type) {
+	case USRQUOTA:
+		return XFS_DQ_USER;
+	case GRPQUOTA:
+		return XFS_DQ_GROUP;
+	default:
+		return XFS_DQ_PROJ;
+	}
+}
+
 /*
  * This is called to free all the memory associated with a dquot
  */
@@ -702,20 +715,21 @@ int
 xfs_qm_dqget(
 	xfs_mount_t	*mp,
 	xfs_inode_t	*ip,	  /* locked inode (optional) */
-	xfs_dqid_t	id,	  /* uid/projid/gid depending on type */
-	uint		type,	  /* XFS_DQ_USER/XFS_DQ_PROJ/XFS_DQ_GROUP */
+	struct kqid     qid,	  /* uid/projid/gid depending on type */
 	uint		flags,	  /* DQALLOC, DQSUSER, DQREPAIR, DOWARN */
 	xfs_dquot_t	**O_dqpp) /* OUT : locked incore dquot */
 {
 	struct xfs_quotainfo	*qi = mp->m_quotainfo;
+	uint			id = from_kqid(&init_user_ns, qid);
+	uint			type = xfs_quota_type(qid.type);
 	struct radix_tree_root *tree = XFS_DQUOT_TREE(qi, type);
 	struct xfs_dquot	*dqp;
 	int			error;
 
 	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
-	if ((! XFS_IS_UQUOTA_ON(mp) && type == XFS_DQ_USER) ||
-	    (! XFS_IS_PQUOTA_ON(mp) && type == XFS_DQ_PROJ) ||
-	    (! XFS_IS_GQUOTA_ON(mp) && type == XFS_DQ_GROUP)) {
+	if ((! XFS_IS_UQUOTA_ON(mp) && qid.type == USRQUOTA) ||
+	    (! XFS_IS_PQUOTA_ON(mp) && qid.type == GRPQUOTA) ||
+	    (! XFS_IS_GQUOTA_ON(mp) && qid.type == PRJQUOTA)) {
 		return (ESRCH);
 	}
 
diff --git a/fs/xfs/xfs_dquot.h b/fs/xfs/xfs_dquot.h
index 2c197da..3566548 100644
--- a/fs/xfs/xfs_dquot.h
+++ b/fs/xfs/xfs_dquot.h
@@ -147,8 +147,8 @@ extern void		xfs_qm_adjust_dqtimers(xfs_mount_t *,
 					xfs_disk_dquot_t *);
 extern void		xfs_qm_adjust_dqlimits(xfs_mount_t *,
 					xfs_disk_dquot_t *);
-extern int		xfs_qm_dqget(xfs_mount_t *, xfs_inode_t *,
-					xfs_dqid_t, uint, uint, xfs_dquot_t **);
+extern int		xfs_qm_dqget(xfs_mount_t *, xfs_inode_t *, struct kqid,
+					uint, xfs_dquot_t **);
 extern void		xfs_qm_dqput(xfs_dquot_t *);
 
 extern void		xfs_dqlock2(struct xfs_dquot *, struct xfs_dquot *);
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 80b8c81..836d40d 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -367,8 +367,7 @@ xfs_qm_unmount_quotas(
 STATIC int
 xfs_qm_dqattach_one(
 	xfs_inode_t	*ip,
-	xfs_dqid_t	id,
-	uint		type,
+	struct kqid	id,
 	uint		doalloc,
 	xfs_dquot_t	*udqhint, /* hint */
 	xfs_dquot_t	**IO_idqpp)
@@ -397,7 +396,7 @@ xfs_qm_dqattach_one(
 	 * the user dquot.
 	 */
 	if (udqhint) {
-		ASSERT(type == XFS_DQ_GROUP || type == XFS_DQ_PROJ);
+		ASSERT(id.type == GRPQUOTA || id.type == PRJQUOTA);
 		xfs_dqlock(udqhint);
 
 		/*
@@ -408,7 +407,7 @@ xfs_qm_dqattach_one(
 		 * hold the ilock.
 		 */
 		dqp = udqhint->q_gdquot;
-		if (dqp && be32_to_cpu(dqp->q_core.d_id) == id) {
+		if (dqp && qid_eq(make_kqid(&init_user_ns, id.type, be32_to_cpu(dqp->q_core.d_id)), id)) {
 			ASSERT(*IO_idqpp == NULL);
 
 			*IO_idqpp = xfs_qm_dqhold(dqp);
@@ -432,7 +431,7 @@ xfs_qm_dqattach_one(
 	 * disk and we didn't ask it to allocate;
 	 * ESRCH if quotas got turned off suddenly.
 	 */
-	error = xfs_qm_dqget(ip->i_mount, ip, id, type,
+	error = xfs_qm_dqget(ip->i_mount, ip, id,
 			     doalloc | XFS_QMOPT_DOWARN, &dqp);
 	if (error)
 		return error;
@@ -516,7 +515,7 @@ xfs_qm_dqattach_locked(
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 
 	if (XFS_IS_UQUOTA_ON(mp)) {
-		error = xfs_qm_dqattach_one(ip, VFS_I(ip)->i_uid, XFS_DQ_USER,
+		error = xfs_qm_dqattach_one(ip, make_kqid_uid(VFS_I(ip)->i_uid),
 						flags & XFS_QMOPT_DQALLOC,
 						NULL, &ip->i_udquot);
 		if (error)
@@ -527,10 +526,10 @@ xfs_qm_dqattach_locked(
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 	if (XFS_IS_OQUOTA_ON(mp)) {
 		error = XFS_IS_GQUOTA_ON(mp) ?
-			xfs_qm_dqattach_one(ip, VFS_I(ip)->i_gid, XFS_DQ_GROUP,
+			xfs_qm_dqattach_one(ip, make_kqid_gid(VFS_I(ip)->i_gid),
 						flags & XFS_QMOPT_DQALLOC,
 						ip->i_udquot, &ip->i_gdquot) :
-			xfs_qm_dqattach_one(ip, ip->i_projid, XFS_DQ_PROJ,
+			xfs_qm_dqattach_one(ip, make_kqid_projid(ip->i_projid),
 						flags & XFS_QMOPT_DQALLOC,
 						ip->i_udquot, &ip->i_gdquot);
 		/*
@@ -1016,8 +1015,7 @@ out:
 STATIC int
 xfs_qm_quotacheck_dqadjust(
 	struct xfs_inode	*ip,
-	xfs_dqid_t		id,
-	uint			type,
+	struct kqid		id,
 	xfs_qcnt_t		nblks,
 	xfs_qcnt_t		rtblks)
 {
@@ -1025,7 +1023,7 @@ xfs_qm_quotacheck_dqadjust(
 	struct xfs_dquot	*dqp;
 	int			error;
 
-	error = xfs_qm_dqget(mp, ip, id, type,
+	error = xfs_qm_dqget(mp, ip, id,
 			     XFS_QMOPT_DQALLOC | XFS_QMOPT_DOWARN, &dqp);
 	if (error) {
 		/*
@@ -1160,22 +1158,22 @@ xfs_qm_dqusage_adjust(
 	 * and quotaoffs don't race. (Quotachecks happen at mount time only).
 	 */
 	if (XFS_IS_UQUOTA_ON(mp)) {
-		error = xfs_qm_quotacheck_dqadjust(ip, VFS_I(ip)->i_uid,
-						   XFS_DQ_USER, nblks, rtblks);
+		error = xfs_qm_quotacheck_dqadjust(ip, make_kqid_uid(VFS_I(ip)->i_uid),
+						   nblks, rtblks);
 		if (error)
 			goto error0;
 	}
 
 	if (XFS_IS_GQUOTA_ON(mp)) {
-		error = xfs_qm_quotacheck_dqadjust(ip, VFS_I(ip)->i_gid,
-						   XFS_DQ_GROUP, nblks, rtblks);
+		error = xfs_qm_quotacheck_dqadjust(ip, make_kqid_gid(VFS_I(ip)->i_gid),
+						   nblks, rtblks);
 		if (error)
 			goto error0;
 	}
 
 	if (XFS_IS_PQUOTA_ON(mp)) {
-		error = xfs_qm_quotacheck_dqadjust(ip, ip->i_projid,
-						   XFS_DQ_PROJ, nblks, rtblks);
+		error = xfs_qm_quotacheck_dqadjust(ip, make_kqid_projid(ip->i_projid),
+						   nblks, rtblks);
 		if (error)
 			goto error0;
 	}
@@ -1663,8 +1661,7 @@ xfs_qm_vop_dqalloc(
 			 * holding ilock.
 			 */
 			xfs_iunlock(ip, lockflags);
-			if ((error = xfs_qm_dqget(mp, NULL, (xfs_dqid_t) uid,
-						 XFS_DQ_USER,
+			if ((error = xfs_qm_dqget(mp, NULL, make_kqid_uid(uid),
 						 XFS_QMOPT_DQALLOC |
 						 XFS_QMOPT_DOWARN,
 						 &uq))) {
@@ -1689,8 +1686,7 @@ xfs_qm_vop_dqalloc(
 	if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
 		if (!gid_eq(VFS_I(ip)->i_gid, gid)) {
 			xfs_iunlock(ip, lockflags);
-			if ((error = xfs_qm_dqget(mp, NULL, (xfs_dqid_t)gid,
-						 XFS_DQ_GROUP,
+			if ((error = xfs_qm_dqget(mp, NULL, make_kqid_gid(gid),
 						 XFS_QMOPT_DQALLOC |
 						 XFS_QMOPT_DOWARN,
 						 &gq))) {
@@ -1709,8 +1705,8 @@ xfs_qm_vop_dqalloc(
 	} else if ((flags & XFS_QMOPT_PQUOTA) && XFS_IS_PQUOTA_ON(mp)) {
 		if (!projid_eq(ip->i_projid, prid)) {
 			xfs_iunlock(ip, lockflags);
-			if ((error = xfs_qm_dqget(mp, NULL, (xfs_dqid_t)prid,
-						 XFS_DQ_PROJ,
+			if ((error = xfs_qm_dqget(mp, NULL,
+						 make_kqid_projid(prid),
 						 XFS_QMOPT_DQALLOC |
 						 XFS_QMOPT_DOWARN,
 						 &gq))) {
diff --git a/fs/xfs/xfs_qm_bhv.c b/fs/xfs/xfs_qm_bhv.c
index 3abac1b..aceb81e 100644
--- a/fs/xfs/xfs_qm_bhv.c
+++ b/fs/xfs/xfs_qm_bhv.c
@@ -79,7 +79,7 @@ xfs_qm_statvfs(
 	xfs_mount_t		*mp = ip->i_mount;
 	xfs_dquot_t		*dqp;
 
-	if (!xfs_qm_dqget(mp, NULL, ip->i_projid, XFS_DQ_PROJ, 0, &dqp)) {
+	if (!xfs_qm_dqget(mp, NULL, make_kqid_projid(ip->i_projid), 0, &dqp)) {
 		xfs_fill_statvfs_from_dquot(statp, dqp);
 		xfs_qm_dqput(dqp);
 	}
diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index 5666b1c..90f6255 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -48,19 +48,6 @@ STATIC int	xfs_qm_log_quotaoff_end(xfs_mount_t *, xfs_qoff_logitem_t *,
 STATIC uint	xfs_qm_export_flags(uint);
 STATIC uint	xfs_qm_export_qtype_flags(uint);
 
-STATIC int
-xfs_quota_type(int type)
-{
-	switch (type) {
-	case USRQUOTA:
-		return XFS_DQ_USER;
-	case GRPQUOTA:
-		return XFS_DQ_GROUP;
-	default:
-		return XFS_DQ_PROJ;
-	}
-}
-
 /*
  * Turn off quota accounting and/or enforcement for all udquots and/or
  * gdquots. Called only at unmount time.
@@ -524,9 +511,7 @@ xfs_qm_scall_setqlim(
 	 * Get the dquot (locked), and join it to the transaction.
 	 * Allocate the dquot if this doesn't exist.
 	 */
-	if ((error = xfs_qm_dqget(mp, NULL, from_kqid(&init_user_ns, id),
-				  xfs_quota_type(id.type),
-				  XFS_QMOPT_DQALLOC, &dqp))) {
+	if ((error = xfs_qm_dqget(mp, NULL, id, XFS_QMOPT_DQALLOC, &dqp))) {
 		xfs_trans_cancel(tp, XFS_TRANS_ABORT);
 		ASSERT(error != ENOENT);
 		goto out_unlock;
@@ -746,8 +731,7 @@ xfs_qm_scall_getquota(
 	 * we aren't passing the XFS_QMOPT_DOALLOC flag. If it doesn't
 	 * exist, we'll get ENOENT back.
 	 */
-	error = xfs_qm_dqget(mp, NULL, from_kqid(&init_user_ns, id),
-			     xfs_quota_type(id.type), 0, &dqp);
+	error = xfs_qm_dqget(mp, NULL, id, 0, &dqp);
 	if (error)
 		return error;
 
-- 
1.7.5.4

[PATCH review 13/16] xfs: Use q_id instead of q_core.d_id.

[Eric W. Biederman]

From: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>

- Use qid_lt on q_id when compariting two quota entries for order.
- Use qid_eq or !qid_eq when comparing a quota entry for equality or inequality.
- Use is_superquota when testing for the magic quota id of 0.
- Use make_kqid_uid, make_kqid_gid, and make_kqid_projid when comparing
  a quota id against a uids, gids, or projids as appropriate.
- For tracing use from_kqid and map to the init user namespace.
  Tracers outside of the initial user namespace are not allowed.

- For generating ondisk values continue to use q_core.d_id.
- For cases where we want an index and don't care which quota
  it is continue to use be32_to_cpu(...->q_core.d_id)

Cc: Ben Myers <bpm-sJ/iWh9BUns@public.gmane.org>
Cc: Alex Elder <elder-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org>
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
 fs/xfs/xfs_dquot.c       |    3 +--
 fs/xfs/xfs_qm.c          |   18 +++++++++---------
 fs/xfs/xfs_trace.h       |    2 +-
 fs/xfs/xfs_trans_dquot.c |    8 ++------
 4 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 53c8f67..6f6e6ea 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -1092,8 +1092,7 @@ xfs_dqlock2(
 {
 	if (d1 && d2) {
 		ASSERT(d1 != d2);
-		if (be32_to_cpu(d1->q_core.d_id) >
-		    be32_to_cpu(d2->q_core.d_id)) {
+		if (qid_lt(d2->q_id, d1->q_id)) {
 			mutex_lock(&d2->q_qlock);
 			mutex_lock_nested(&d1->q_qlock, XFS_QLOCK_NESTED);
 		} else {
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 7e19147..60444b0 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -407,7 +407,7 @@ xfs_qm_dqattach_one(
 		 * hold the ilock.
 		 */
 		dqp = udqhint->q_gdquot;
-		if (dqp && qid_eq(make_kqid(&init_user_ns, id.type, be32_to_cpu(dqp->q_core.d_id)), id)) {
+		if (dqp && qid_eq(dqp->q_id, id)) {
 			ASSERT(*IO_idqpp == NULL);
 
 			*IO_idqpp = xfs_qm_dqhold(dqp);
@@ -1057,7 +1057,7 @@ xfs_qm_quotacheck_dqadjust(
 	 *
 	 * There are no timers for the default values set in the root dquot.
 	 */
-	if (dqp->q_core.d_id) {
+	if (!is_superquota(dqp->q_id, &init_user_ns)) {
 		xfs_qm_adjust_dqlimits(mp, &dqp->q_core);
 		xfs_qm_adjust_dqtimers(mp, &dqp->q_core);
 	}
@@ -1805,7 +1805,7 @@ xfs_qm_vop_chown_reserve(
 			XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS;
 
 	if (XFS_IS_UQUOTA_ON(mp) && udqp &&
-	    !uid_eq(VFS_I(ip)->i_uid, (uid_t)be32_to_cpu(udqp->q_core.d_id))) {
+	    !qid_eq(make_kqid_uid(VFS_I(ip)->i_uid), udqp->q_id)) {
 		delblksudq = udqp;
 		/*
 		 * If there are delayed allocation blocks, then we have to
@@ -1819,12 +1819,12 @@ xfs_qm_vop_chown_reserve(
 	}
 	if (XFS_IS_OQUOTA_ON(ip->i_mount) && gdqp) {
 		if (XFS_IS_PQUOTA_ON(ip->i_mount) &&
-		    !projid_eq(ip->i_projid, be32_to_cpu(gdqp->q_core.d_id)))
+		    !qid_eq(make_kqid_projid(ip->i_projid), gdqp->q_id))
 			prjflags = XFS_QMOPT_ENOSPC;
 
 		if (prjflags ||
 		    (XFS_IS_GQUOTA_ON(ip->i_mount) &&
-		     !gid_eq(VFS_I(ip)->i_gid, be32_to_cpu(gdqp->q_core.d_id)))) {
+		     !qid_eq(make_kqid_gid(VFS_I(ip)->i_gid), gdqp->q_id))) {
 			delblksgdq = gdqp;
 			if (delblks) {
 				ASSERT(ip->i_gdquot);
@@ -1908,7 +1908,7 @@ xfs_qm_vop_create_dqattach(
 	if (udqp) {
 		ASSERT(ip->i_udquot == NULL);
 		ASSERT(XFS_IS_UQUOTA_ON(mp));
-		ASSERT(uid_eq(VFS_I(ip)->i_uid, be32_to_cpu(udqp->q_core.d_id)));
+		ASSERT(qid_eq(make_kqid_uid(VFS_I(ip)->i_uid), udqp->q_id));
 
 		ip->i_udquot = xfs_qm_dqhold(udqp);
 		xfs_trans_mod_dquot(tp, udqp, XFS_TRANS_DQ_ICOUNT, 1);
@@ -1916,9 +1916,9 @@ xfs_qm_vop_create_dqattach(
 	if (gdqp) {
 		ASSERT(ip->i_gdquot == NULL);
 		ASSERT(XFS_IS_OQUOTA_ON(mp));
-		ASSERT(XFS_IS_GQUOTA_ON(mp) ?
-		       gid_eq(VFS_I(ip)->i_gid, be32_to_cpu(gdqp->q_core.d_id)):
-		       projid_eq(ip->i_projid, be32_to_cpu(gdqp->q_core.d_id)));
+		ASSERT(qid_eq(XFS_IS_GQUOTA_ON(mp) ?
+			      make_kqid_gid(VFS_I(ip)->i_gid) :
+			      make_kqid_projid(ip->i_projid), gdqp->q_id));
 
 		ip->i_gdquot = xfs_qm_dqhold(gdqp);
 		xfs_trans_mod_dquot(tp, gdqp, XFS_TRANS_DQ_ICOUNT, 1);
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 2e137d4..3cda9b4 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -712,7 +712,7 @@ DECLARE_EVENT_CLASS(xfs_dquot_class,
 	), \
 	TP_fast_assign(
 		__entry->dev = dqp->q_mount->m_super->s_dev;
-		__entry->id = be32_to_cpu(dqp->q_core.d_id);
+		__entry->id = from_kqid(&init_user_ns, dqp->q_id);
 		__entry->flags = dqp->dq_flags;
 		__entry->nrefs = dqp->q_nrefs;
 		__entry->res_bcount = dqp->q_res_bcount;
diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index 0c7fa54..5e5fdfe 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -578,11 +578,7 @@ xfs_quota_warn(
 	/* no warnings for project quotas - we just return ENOSPC later */
 	if (dqp->dq_flags & XFS_DQ_PROJ)
 		return;
-	quota_send_warning(make_kqid(&init_user_ns,
-				     (dqp->dq_flags & XFS_DQ_USER) ?
-				     USRQUOTA : GRPQUOTA,
-				     be32_to_cpu(dqp->q_core.d_id)),
-			   mp->m_super->s_dev, type);
+	quota_send_warning(dqp->q_id, mp->m_super->s_dev, type);
 }
 
 /*
@@ -638,7 +634,7 @@ xfs_trans_dqresv(
 	}
 
 	if ((flags & XFS_QMOPT_FORCE_RES) == 0 &&
-	    dqp->q_core.d_id &&
+	    !is_superquota(dqp->q_id, &init_user_ns) &&
 	    ((XFS_IS_UQUOTA_ENFORCED(dqp->q_mount) && XFS_QM_ISUDQ(dqp)) ||
 	     (XFS_IS_OQUOTA_ENFORCED(dqp->q_mount) &&
 	      (XFS_QM_ISPDQ(dqp) || XFS_QM_ISGDQ(dqp))))) {
-- 
1.7.5.4

[PATCH review 14/16] xfs: Enable building with user namespaces enabled.

[Eric W. Biederman]

From: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>

Not that all of the types holding uids, gids, and projectids
have been converted it is safe to allow users in different
user namespaces to access an xfs filesystem.

Cc: Ben Myers <bpm-sJ/iWh9BUns@public.gmane.org>
Cc: Alex Elder <elder-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org>
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
 init/Kconfig |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index b2de5ed..3d99394 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1071,7 +1071,6 @@ config UIDGID_CONVERTED
 	default y
 
 	# Filesystems
-	depends on XFS_FS = n
 
 config UIDGID_STRICT_TYPE_CHECKS
 	bool "Require conversions between uid/gids and their internal representation"
-- 
1.7.5.4

[PATCH review 08/16] xfs: Use kprojids when allocating inodes.

[Eric W. Biederman]

From: "Eric W. Biederman" <ebiederm@xmission.com>

In xfs_create and xfs_symlink compute the desired kprojid and pass it
down into xfs_ialloc.

Cc: Ben Myers <bpm@sgi.com>
Cc: Alex Elder <elder@kernel.org>
Cc: Dave Chinner <david@fromorbit.com>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/xfs/xfs_inode.c    |    5 +++--
 fs/xfs/xfs_inode.h    |    2 +-
 fs/xfs/xfs_qm.c       |    4 +++-
 fs/xfs/xfs_utils.c    |    2 +-
 fs/xfs/xfs_utils.h    |    2 +-
 fs/xfs/xfs_vnodeops.c |    8 ++++----
 6 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index af5420a..b7c9975 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1163,7 +1163,7 @@ xfs_ialloc(
 	umode_t		mode,
 	xfs_nlink_t	nlink,
 	xfs_dev_t	rdev,
-	prid_t		prid,
+	kprojid_t	prid,
 	int		okalloc,
 	xfs_buf_t	**ialloc_context,
 	xfs_inode_t	**ipp)
@@ -1227,7 +1227,8 @@ xfs_ialloc(
 	/*
 	 * Project ids won't be stored on disk if we are using a version 1 inode.
 	 */
-	if ((prid != 0) && (ip->i_d.di_version == 1))
+	if (!projid_eq(prid, make_kprojid(&init_user_ns, 0)) &&
+	    (ip->i_d.di_version == 1))
 		xfs_bump_ino_vers2(tp, ip);
 
 	if (pip && XFS_INHERIT_GID(pip)) {
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index b82160f..9232c8e 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -518,7 +518,7 @@ int		xfs_isilocked(xfs_inode_t *, uint);
 uint		xfs_ilock_map_shared(xfs_inode_t *);
 void		xfs_iunlock_map_shared(xfs_inode_t *, uint);
 int		xfs_ialloc(struct xfs_trans *, xfs_inode_t *, umode_t,
-			   xfs_nlink_t, xfs_dev_t, prid_t, int,
+			   xfs_nlink_t, xfs_dev_t, kprojid_t, int,
 			   struct xfs_buf **, xfs_inode_t **);
 
 uint		xfs_ip2xflags(struct xfs_inode *);
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index f11e10d..6fce3d3 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -777,7 +777,9 @@ xfs_qm_qino_alloc(
 		return error;
 	}
 
-	error = xfs_dir_ialloc(&tp, NULL, S_IFREG, 1, 0, 0, 1, ip, &committed);
+	error = xfs_dir_ialloc(&tp, NULL, S_IFREG, 1, 0,
+			       make_kprojid(&init_user_ns, 0),
+			       1, ip, &committed);
 	if (error) {
 		xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES |
 				 XFS_TRANS_ABORT);
diff --git a/fs/xfs/xfs_utils.c b/fs/xfs/xfs_utils.c
index 0025c78..f0a9f1d 100644
--- a/fs/xfs/xfs_utils.c
+++ b/fs/xfs/xfs_utils.c
@@ -54,7 +54,7 @@ xfs_dir_ialloc(
 	umode_t		mode,
 	xfs_nlink_t	nlink,
 	xfs_dev_t	rdev,
-	prid_t		prid,		/* project id */
+	kprojid_t	prid,		/* project id */
 	int		okalloc,	/* ok to allocate new space */
 	xfs_inode_t	**ipp,		/* pointer to inode; it will be
 					   locked. */
diff --git a/fs/xfs/xfs_utils.h b/fs/xfs/xfs_utils.h
index 5eeab46..7757f7c 100644
--- a/fs/xfs/xfs_utils.h
+++ b/fs/xfs/xfs_utils.h
@@ -19,7 +19,7 @@
 #define __XFS_UTILS_H__
 
 extern int xfs_dir_ialloc(xfs_trans_t **, xfs_inode_t *, umode_t, xfs_nlink_t,
-				xfs_dev_t, prid_t, int, xfs_inode_t **, int *);
+				xfs_dev_t, kprojid_t, int, xfs_inode_t **, int *);
 extern int xfs_droplink(xfs_trans_t *, xfs_inode_t *);
 extern int xfs_bumplink(xfs_trans_t *, xfs_inode_t *);
 extern void xfs_bump_ino_vers2(xfs_trans_t *, xfs_inode_t *);
diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
index e7b27c8..496460b 100644
--- a/fs/xfs/xfs_vnodeops.c
+++ b/fs/xfs/xfs_vnodeops.c
@@ -728,7 +728,7 @@ xfs_create(
 	boolean_t		unlock_dp_on_error = B_FALSE;
 	uint			cancel_flags;
 	int			committed;
-	prid_t			prid;
+	kprojid_t		prid;
 	struct xfs_dquot	*udqp = NULL;
 	struct xfs_dquot	*gdqp = NULL;
 	uint			resblks;
@@ -743,7 +743,7 @@ xfs_create(
 	if (dp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT)
 		prid = dp->i_projid;
 	else
-		prid = XFS_PROJID_DEFAULT;
+		prid = make_kprojid(&init_user_ns, XFS_PROJID_DEFAULT);
 
 	/*
 	 * Make sure that we have allocated dquot(s) on disk.
@@ -1379,7 +1379,7 @@ xfs_symlink(
 	int			byte_cnt;
 	int			n;
 	xfs_buf_t		*bp;
-	prid_t			prid;
+	kprojid_t		prid;
 	struct xfs_dquot	*udqp, *gdqp;
 	uint			resblks;
 
@@ -1404,7 +1404,7 @@ xfs_symlink(
 	if (dp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT)
 		prid = dp->i_projid;
 	else
-		prid = XFS_PROJID_DEFAULT;
+		prid = make_kprojid(&init_user_ns, XFS_PROJID_DEFAULT);
 
 	/*
 	 * Make sure that we have allocated dquot(s) on disk.
-- 
1.7.5.4

Re: [PATCH review 08/16] xfs: Use kprojids when allocating inodes.

[Dave Chinner]

On Sun, Feb 17, 2013 at 05:11:01PM -0800, Eric W. Biederman wrote:
> From: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
> 
> In xfs_create and xfs_symlink compute the desired kprojid and pass it
> down into xfs_ialloc.

NACK.

The first time you posted this code I NACKed it because:

>> This sort of thing just makes me cringe. This is all internal
>> project ID management that has nothing to do with namespaces.
>> It's for project ID's that are inherited from the parent inode,
>> and as such we do not care one bit what the namespace is.
>> Internal passing of project IDs like this this should not be
>> converted at all as it has nothing at all to do with the
>> namespaces.

Please drop this patch or replace it with a simple patch that passes
the project ID as an xfs_dqid_t (i.e. a flat, 32 bit quota
identifier) instead so you can kill the prid_t type.

Cheers,

Dave.
-- 
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org

[PATCH review 09/16] xfs: Modify xfs_qm_vop_dqalloc to take kuids, kgids, and kprojids.

[Eric W. Biederman]

From: "Eric W. Biederman" <ebiederm@xmission.com>

Cc: Ben Myers <bpm@sgi.com>
Cc: Alex Elder <elder@kernel.org>
Cc: Dave Chinner <david@fromorbit.com>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/xfs/xfs_qm.c    |    6 +++---
 fs/xfs/xfs_quota.h |    4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 6fce3d3..80b8c81 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -1617,9 +1617,9 @@ xfs_qm_write_sb_changes(
 int
 xfs_qm_vop_dqalloc(
 	struct xfs_inode	*ip,
-	uid_t			uid,
-	gid_t			gid,
-	prid_t			prid,
+	kuid_t			uid,
+	kgid_t			gid,
+	kprojid_t		prid,
 	uint			flags,
 	struct xfs_dquot	**O_udqpp,
 	struct xfs_dquot	**O_gdqpp)
diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h
index b50ec5b..f48f801 100644
--- a/fs/xfs/xfs_quota.h
+++ b/fs/xfs/xfs_quota.h
@@ -311,7 +311,7 @@ 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,
+extern int xfs_qm_vop_dqalloc(struct xfs_inode *, kuid_t, kgid_t, kprojid_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 *);
@@ -332,7 +332,7 @@ 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,
+xfs_qm_vop_dqalloc(struct xfs_inode *ip, kuid_t uid, kgid_t gid, kprojid_t prid,
 		uint flags, struct xfs_dquot **udqp, struct xfs_dquot **gdqp)
 {
 	*udqp = NULL;
-- 
1.7.5.4

Re: [PATCH review 09/16] xfs: Modify xfs_qm_vop_dqalloc to take kuids, kgids, and kprojids.

[Dave Chinner]

On Sun, Feb 17, 2013 at 05:11:02PM -0800, Eric W. Biederman wrote:
> From: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
> 
> Cc: Ben Myers <bpm-sJ/iWh9BUns@public.gmane.org>
> Cc: Alex Elder <elder-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org>
> Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
> ---
>  fs/xfs/xfs_qm.c    |    6 +++---
>  fs/xfs/xfs_quota.h |    4 ++--
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 6fce3d3..80b8c81 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -1617,9 +1617,9 @@ xfs_qm_write_sb_changes(
>  int
>  xfs_qm_vop_dqalloc(
>  	struct xfs_inode	*ip,
> -	uid_t			uid,
> -	gid_t			gid,
> -	prid_t			prid,
> +	kuid_t			uid,
> +	kgid_t			gid,
> +	kprojid_t		prid,
>  	uint			flags,
>  	struct xfs_dquot	**O_udqpp,
>  	struct xfs_dquot	**O_gdqpp)

Probably they should use xfs_dqid_t as they are supposed to be
internal XFS quota ID types....

Cheers,

Dave.
-- 
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org

[PATCH review 10/16] xfs: Push struct kqid into xfs_qm_scall_qmlim and xfs_qm_scall_getquota

[Eric W. Biederman]

From: "Eric W. Biederman" <ebiederm@xmission.com>

- In xfs_qm_scall_getquota map the quota id into the callers
  user namespace in the returned struct fs_disk_quota

- Add a helper is_superquota and use it in xfs_qm_scall_setqlimi
  to see if we are setting the superusers quota limit.  Setting
  the superuses quota limit on xfs sets the default quota limits
  for all users.

- Move xfs_quota_type into xfs_qm_syscalls.c where it is now used.

Cc: Ben Myers <bpm@sgi.com>
Cc: Alex Elder <elder@kernel.org>
Cc: Dave Chinner <david@fromorbit.com>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/xfs/xfs_dquot.h       |    6 ++++++
 fs/xfs/xfs_qm.h          |    4 ++--
 fs/xfs/xfs_qm_syscalls.c |   40 +++++++++++++++++++++++++++++-----------
 fs/xfs/xfs_quotaops.c    |   20 ++------------------
 4 files changed, 39 insertions(+), 31 deletions(-)

diff --git a/fs/xfs/xfs_dquot.h b/fs/xfs/xfs_dquot.h
index c694a84..2c197da 100644
--- a/fs/xfs/xfs_dquot.h
+++ b/fs/xfs/xfs_dquot.h
@@ -163,4 +163,10 @@ static inline struct xfs_dquot *xfs_qm_dqhold(struct xfs_dquot *dqp)
 
 extern const struct xfs_buf_ops xfs_dquot_buf_ops;
 
+/* Is this root's quota in the specified user namespace? */
+static inline bool is_superquota(struct kqid id, struct user_namespace *ns)
+{
+	return qid_eq(id, make_kqid(ns, id.type, 0));
+}
+
 #endif /* __XFS_DQUOT_H__ */
diff --git a/fs/xfs/xfs_qm.h b/fs/xfs/xfs_qm.h
index 44b858b..ce478dc 100644
--- a/fs/xfs/xfs_qm.h
+++ b/fs/xfs/xfs_qm.h
@@ -114,9 +114,9 @@ extern void		xfs_qm_dqrele_all_inodes(xfs_mount_t *, uint);
 
 /* quota ops */
 extern int		xfs_qm_scall_trunc_qfiles(xfs_mount_t *, uint);
-extern int		xfs_qm_scall_getquota(xfs_mount_t *, xfs_dqid_t, uint,
+extern int		xfs_qm_scall_getquota(xfs_mount_t *, struct kqid,
 					fs_disk_quota_t *);
-extern int		xfs_qm_scall_setqlim(xfs_mount_t *, xfs_dqid_t, uint,
+extern int		xfs_qm_scall_setqlim(xfs_mount_t *, struct kqid,
 					fs_disk_quota_t *);
 extern int		xfs_qm_scall_getqstat(xfs_mount_t *, fs_quota_stat_t *);
 extern int		xfs_qm_scall_quotaon(xfs_mount_t *, uint);
diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index 5f53e75..5666b1c 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -48,6 +48,19 @@ STATIC int	xfs_qm_log_quotaoff_end(xfs_mount_t *, xfs_qoff_logitem_t *,
 STATIC uint	xfs_qm_export_flags(uint);
 STATIC uint	xfs_qm_export_qtype_flags(uint);
 
+STATIC int
+xfs_quota_type(int type)
+{
+	switch (type) {
+	case USRQUOTA:
+		return XFS_DQ_USER;
+	case GRPQUOTA:
+		return XFS_DQ_GROUP;
+	default:
+		return XFS_DQ_PROJ;
+	}
+}
+
 /*
  * Turn off quota accounting and/or enforcement for all udquots and/or
  * gdquots. Called only at unmount time.
@@ -473,8 +486,7 @@ xfs_qm_scall_getqstat(
 int
 xfs_qm_scall_setqlim(
 	xfs_mount_t		*mp,
-	xfs_dqid_t		id,
-	uint			type,
+	struct kqid		id,
 	fs_disk_quota_t		*newlim)
 {
 	struct xfs_quotainfo	*q = mp->m_quotainfo;
@@ -483,12 +495,16 @@ xfs_qm_scall_setqlim(
 	xfs_trans_t		*tp;
 	int			error;
 	xfs_qcnt_t		hard, soft;
+	bool			superquota;
 
 	if (newlim->d_fieldmask & ~XFS_DQ_MASK)
 		return EINVAL;
 	if ((newlim->d_fieldmask & XFS_DQ_MASK) == 0)
 		return 0;
 
+	/* Is this quota id for root? */
+	superquota = is_superquota(id, &init_user_ns);
+
 	tp = xfs_trans_alloc(mp, XFS_TRANS_QM_SETQLIM);
 	if ((error = xfs_trans_reserve(tp, 0, sizeof(xfs_disk_dquot_t) + 128,
 				      0, 0, XFS_DEFAULT_LOG_COUNT))) {
@@ -508,7 +524,9 @@ xfs_qm_scall_setqlim(
 	 * Get the dquot (locked), and join it to the transaction.
 	 * Allocate the dquot if this doesn't exist.
 	 */
-	if ((error = xfs_qm_dqget(mp, NULL, id, type, XFS_QMOPT_DQALLOC, &dqp))) {
+	if ((error = xfs_qm_dqget(mp, NULL, from_kqid(&init_user_ns, id),
+				  xfs_quota_type(id.type),
+				  XFS_QMOPT_DQALLOC, &dqp))) {
 		xfs_trans_cancel(tp, XFS_TRANS_ABORT);
 		ASSERT(error != ENOENT);
 		goto out_unlock;
@@ -528,7 +546,7 @@ xfs_qm_scall_setqlim(
 	if (hard == 0 || hard >= soft) {
 		ddq->d_blk_hardlimit = cpu_to_be64(hard);
 		ddq->d_blk_softlimit = cpu_to_be64(soft);
-		if (id == 0) {
+		if (superquota) {
 			q->qi_bhardlimit = hard;
 			q->qi_bsoftlimit = soft;
 		}
@@ -544,7 +562,7 @@ xfs_qm_scall_setqlim(
 	if (hard == 0 || hard >= soft) {
 		ddq->d_rtb_hardlimit = cpu_to_be64(hard);
 		ddq->d_rtb_softlimit = cpu_to_be64(soft);
-		if (id == 0) {
+		if (superquota) {
 			q->qi_rtbhardlimit = hard;
 			q->qi_rtbsoftlimit = soft;
 		}
@@ -561,7 +579,7 @@ xfs_qm_scall_setqlim(
 	if (hard == 0 || hard >= soft) {
 		ddq->d_ino_hardlimit = cpu_to_be64(hard);
 		ddq->d_ino_softlimit = cpu_to_be64(soft);
-		if (id == 0) {
+		if (superquota) {
 			q->qi_ihardlimit = hard;
 			q->qi_isoftlimit = soft;
 		}
@@ -579,7 +597,7 @@ xfs_qm_scall_setqlim(
 	if (newlim->d_fieldmask & FS_DQ_RTBWARNS)
 		ddq->d_rtbwarns = cpu_to_be16(newlim->d_rtbwarns);
 
-	if (id == 0) {
+	if (superquota) {
 		/*
 		 * Timelimits for the super user set the relative time
 		 * the other users can be over quota for this file system.
@@ -717,8 +735,7 @@ error0:
 int
 xfs_qm_scall_getquota(
 	struct xfs_mount	*mp,
-	xfs_dqid_t		id,
-	uint			type,
+	struct kqid		id,
 	struct fs_disk_quota	*dst)
 {
 	struct xfs_dquot	*dqp;
@@ -729,7 +746,8 @@ xfs_qm_scall_getquota(
 	 * we aren't passing the XFS_QMOPT_DOALLOC flag. If it doesn't
 	 * exist, we'll get ENOENT back.
 	 */
-	error = xfs_qm_dqget(mp, NULL, id, type, 0, &dqp);
+	error = xfs_qm_dqget(mp, NULL, from_kqid(&init_user_ns, id),
+			     xfs_quota_type(id.type), 0, &dqp);
 	if (error)
 		return error;
 
@@ -745,7 +763,7 @@ xfs_qm_scall_getquota(
 	memset(dst, 0, sizeof(*dst));
 	dst->d_version = FS_DQUOT_VERSION;
 	dst->d_flags = xfs_qm_export_qtype_flags(dqp->q_core.d_flags);
-	dst->d_id = be32_to_cpu(dqp->q_core.d_id);
+	dst->d_id = from_kqid_munged(current_user_ns(), id);
 	dst->d_blk_hardlimit =
 		XFS_FSB_TO_BB(mp, be64_to_cpu(dqp->q_core.d_blk_hardlimit));
 	dst->d_blk_softlimit =
diff --git a/fs/xfs/xfs_quotaops.c b/fs/xfs/xfs_quotaops.c
index 71926d6..4d88faa 100644
--- a/fs/xfs/xfs_quotaops.c
+++ b/fs/xfs/xfs_quotaops.c
@@ -27,20 +27,6 @@
 #include "xfs_qm.h"
 #include <linux/quota.h>
 
-
-STATIC int
-xfs_quota_type(int type)
-{
-	switch (type) {
-	case USRQUOTA:
-		return XFS_DQ_USER;
-	case GRPQUOTA:
-		return XFS_DQ_GROUP;
-	default:
-		return XFS_DQ_PROJ;
-	}
-}
-
 STATIC int
 xfs_fs_get_xstate(
 	struct super_block	*sb,
@@ -107,8 +93,7 @@ xfs_fs_get_dqblk(
 	if (!XFS_IS_QUOTA_ON(mp))
 		return -ESRCH;
 
-	return -xfs_qm_scall_getquota(mp, from_kqid(&init_user_ns, qid),
-				      xfs_quota_type(qid.type), fdq);
+	return -xfs_qm_scall_getquota(mp, qid, fdq);
 }
 
 STATIC int
@@ -126,8 +111,7 @@ xfs_fs_set_dqblk(
 	if (!XFS_IS_QUOTA_ON(mp))
 		return -ESRCH;
 
-	return -xfs_qm_scall_setqlim(mp, from_kqid(&init_user_ns, qid),
-				     xfs_quota_type(qid.type), fdq);
+	return -xfs_qm_scall_setqlim(mp, qid, fdq);
 }
 
 const struct quotactl_ops xfs_quotactl_operations = {
-- 
1.7.5.4

Re: [PATCH review 10/16] xfs: Push struct kqid into xfs_qm_scall_qmlim and xfs_qm_scall_getquota

[Dave Chinner]

On Sun, Feb 17, 2013 at 05:11:03PM -0800, Eric W. Biederman wrote:
> From: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
> 
> - In xfs_qm_scall_getquota map the quota id into the callers
>   user namespace in the returned struct fs_disk_quota
> 
> - Add a helper is_superquota and use it in xfs_qm_scall_setqlimi
>   to see if we are setting the superusers quota limit.  Setting
>   the superuses quota limit on xfs sets the default quota limits
>   for all users.

These seem fine.

> - Move xfs_quota_type into xfs_qm_syscalls.c where it is now used.

Now that I've seen the code, I really don't see any advantage to
driving the kqid into XFS quota subsystem. (i.e the rest of this
patch and the subsequent follow up patches that drive it further
inward).

I did say previously:

>> From there, targetted patches can drive the kernel structures
>> inward from the entry points where it makes sense to do so (e.g.
>> common places that the quota entry points call that take a
>> type/id pair).  The last thing that should happen is internal
>> structures be converted from type/id pairs to the kernel types if
>> it makes sense to do so and it makes the code simpler and easier
>> to read....

But seeing the result, IMO, it doesn't actually improve the code
(it's neither simpler nor easier to read), and it doesn't actually
add any functionality. It makes the code strange and different and
somewhat inconsistent and litters id/namespace conversions all over
the place, so i don't think these cahgnes are necessary.

Hence I'd say just do absolute minimum needed for the
is_superquota() checks to work and leave all the kqid ->
xfs_dqid_t+type conversion at the boundary of the quota subsystem
where it already is....

Cheers,

Dave.
-- 
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org

[PATCH review 12/16] xfs: Remember the kqid for a quota

[Eric W. Biederman]

From: "Eric W. Biederman" <ebiederm@xmission.com>

- Add q_id to struct xfs_quota
- Modify xfs_qm_dqread to take a struct kqid, allowing xfs_qm_dqread to
  set q_id on fresh quota structures.
- Modify xfs_qm_dqget and xfs_qm_init_quota_info to pass a kqid
  to xfs_qm_dqread

Cc: Ben Myers <bpm@sgi.com>
Cc: Alex Elder <elder@kernel.org>
Cc: Dave Chinner <david@fromorbit.com>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/xfs/xfs_dquot.c |   12 ++++++------
 fs/xfs/xfs_dquot.h |    3 ++-
 fs/xfs/xfs_qm.c    |    9 +++++----
 3 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 6be5a29..53c8f67 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -581,8 +581,7 @@ xfs_qm_dqtobp(
 int
 xfs_qm_dqread(
 	struct xfs_mount	*mp,
-	xfs_dqid_t		id,
-	uint			type,
+	struct kqid		id,
 	uint			flags,
 	struct xfs_dquot	**O_dqpp)
 {
@@ -596,8 +595,9 @@ xfs_qm_dqread(
 
 	dqp = kmem_zone_zalloc(xfs_qm_dqzone, KM_SLEEP);
 
-	dqp->dq_flags = type;
-	dqp->q_core.d_id = cpu_to_be32(id);
+	dqp->q_id = id;
+	dqp->dq_flags = xfs_quota_type(id.type);
+	dqp->q_core.d_id = cpu_to_be32(from_kqid(&init_user_ns, id));
 	dqp->q_mount = mp;
 	INIT_LIST_HEAD(&dqp->q_lru);
 	mutex_init(&dqp->q_qlock);
@@ -615,7 +615,7 @@ xfs_qm_dqread(
 	 * Make sure group quotas have a different lock class than user
 	 * quotas.
 	 */
-	if (!(type & XFS_DQ_USER))
+	if (id.type != USRQUOTA)
 		lockdep_set_class(&dqp->q_qlock, &xfs_dquot_other_class);
 
 	XFS_STATS_INC(xs_qm_dquot);
@@ -785,7 +785,7 @@ restart:
 	if (ip)
 		xfs_iunlock(ip, XFS_ILOCK_EXCL);
 
-	error = xfs_qm_dqread(mp, id, type, flags, &dqp);
+	error = xfs_qm_dqread(mp, qid, flags, &dqp);
 
 	if (ip)
 		xfs_ilock(ip, XFS_ILOCK_EXCL);
diff --git a/fs/xfs/xfs_dquot.h b/fs/xfs/xfs_dquot.h
index 3566548..0b6020e 100644
--- a/fs/xfs/xfs_dquot.h
+++ b/fs/xfs/xfs_dquot.h
@@ -36,6 +36,7 @@ struct xfs_trans;
  * The incore dquot structure
  */
 typedef struct xfs_dquot {
+	struct kqid      q_id;		/* quota identifier */
 	uint		 dq_flags;	/* various flags (XFS_DQ_*) */
 	struct list_head q_lru;		/* global free list of dquots */
 	struct xfs_mount*q_mount;	/* filesystem this relates to */
@@ -138,7 +139,7 @@ static inline xfs_dquot_t *xfs_inode_dquot(struct xfs_inode *ip, int type)
 				 XFS_DQ_TO_QINF(dqp)->qi_uquotaip : \
 				 XFS_DQ_TO_QINF(dqp)->qi_gquotaip)
 
-extern int		xfs_qm_dqread(struct xfs_mount *, xfs_dqid_t, uint,
+extern int		xfs_qm_dqread(struct xfs_mount *, struct kqid,
 					uint, struct xfs_dquot	**);
 extern void		xfs_qm_dqdestroy(xfs_dquot_t *);
 extern int		xfs_qm_dqflush(struct xfs_dquot *, struct xfs_buf **);
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 836d40d..7e19147 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -673,10 +673,11 @@ xfs_qm_init_quotainfo(
 	 * Since we may not have done a quotacheck by this point, just read
 	 * the dquot without attaching it to any hashtables or lists.
 	 */
-	error = xfs_qm_dqread(mp, 0,
-			XFS_IS_UQUOTA_RUNNING(mp) ? XFS_DQ_USER :
-			 (XFS_IS_GQUOTA_RUNNING(mp) ? XFS_DQ_GROUP :
-			  XFS_DQ_PROJ),
+	error = xfs_qm_dqread(mp,
+			make_kqid(&init_user_ns,
+				  XFS_IS_UQUOTA_RUNNING(mp) ? USRQUOTA :
+				  (XFS_IS_GQUOTA_RUNNING(mp) ? GRPQUOTA :
+				   PRJQUOTA), 0),
 			XFS_QMOPT_DOWARN, &dqp);
 	if (!error) {
 		xfs_disk_dquot_t	*ddqp = &dqp->q_core;
-- 
1.7.5.4

[PATCH review 15/16] userns: Now that everything has been converted remove the unnecessary infrastructure

[Eric W. Biederman]

From: "Eric W. Biederman" <ebiederm@xmission.com>

In particular kill UIDGID_CONVERTED and UIDGID_STRICT_TYPE_CHECKS
and have the code treat them as always true.

Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 include/linux/posix_acl.h |    3 ---
 include/linux/projid.h    |   15 ---------------
 include/linux/uidgid.h    |   22 ----------------------
 init/Kconfig              |   23 -----------------------
 4 files changed, 0 insertions(+), 63 deletions(-)

diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h
index 7931efe..f0f7746 100644
--- a/include/linux/posix_acl.h
+++ b/include/linux/posix_acl.h
@@ -39,9 +39,6 @@ struct posix_acl_entry {
 	union {
 		kuid_t		e_uid;
 		kgid_t		e_gid;
-#ifndef CONFIG_UIDGID_STRICT_TYPE_CHECKS
-		unsigned int	e_id;
-#endif
 	};
 };
 
diff --git a/include/linux/projid.h b/include/linux/projid.h
index 36517b9..8c1f2c5 100644
--- a/include/linux/projid.h
+++ b/include/linux/projid.h
@@ -18,8 +18,6 @@ extern struct user_namespace init_user_ns;
 
 typedef __kernel_uid32_t projid_t;
 
-#ifdef CONFIG_UIDGID_STRICT_TYPE_CHECKS
-
 typedef struct {
 	projid_t val;
 } kprojid_t;
@@ -31,19 +29,6 @@ static inline projid_t __kprojid_val(kprojid_t projid)
 
 #define KPROJIDT_INIT(value) (kprojid_t){ value }
 
-#else
-
-typedef projid_t kprojid_t;
-
-static inline projid_t __kprojid_val(kprojid_t projid)
-{
-	return projid;
-}
-
-#define KPROJIDT_INIT(value) ((kprojid_t) value )
-
-#endif
-
 #define INVALID_PROJID KPROJIDT_INIT(-1)
 #define OVERFLOW_PROJID 65534
 
diff --git a/include/linux/uidgid.h b/include/linux/uidgid.h
index 8e522cbc..2d1f9b6 100644
--- a/include/linux/uidgid.h
+++ b/include/linux/uidgid.h
@@ -17,8 +17,6 @@
 struct user_namespace;
 extern struct user_namespace init_user_ns;
 
-#ifdef CONFIG_UIDGID_STRICT_TYPE_CHECKS
-
 typedef struct {
 	uid_t val;
 } kuid_t;
@@ -41,26 +39,6 @@ static inline gid_t __kgid_val(kgid_t gid)
 	return gid.val;
 }
 
-#else
-
-typedef uid_t kuid_t;
-typedef gid_t kgid_t;
-
-static inline uid_t __kuid_val(kuid_t uid)
-{
-	return uid;
-}
-
-static inline gid_t __kgid_val(kgid_t gid)
-{
-	return gid;
-}
-
-#define KUIDT_INIT(value) ((kuid_t) value )
-#define KGIDT_INIT(value) ((kgid_t) value )
-
-#endif
-
 #define GLOBAL_ROOT_UID KUIDT_INIT(0)
 #define GLOBAL_ROOT_GID KGIDT_INIT(0)
 
diff --git a/init/Kconfig b/init/Kconfig
index 3d99394..c22e1b9 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1028,9 +1028,6 @@ config IPC_NS
 config USER_NS
 	bool "User namespace (EXPERIMENTAL)"
 	depends on EXPERIMENTAL
-	depends on UIDGID_CONVERTED
-	select UIDGID_STRICT_TYPE_CHECKS
-
 	default n
 	help
 	  This allows containers, i.e. vservers, to use user namespaces
@@ -1062,26 +1059,6 @@ 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
-
-config UIDGID_STRICT_TYPE_CHECKS
-	bool "Require conversions between uid/gids and their internal representation"
-	depends on UIDGID_CONVERTED
-	default n
-	help
-	 While the nececessary conversions are being added to all subsystems this option allows
-	 the code to continue to build for unconverted subsystems.
-
-	 Say Y here if you want the strict type checking enabled
-
 config SCHED_AUTOGROUP
 	bool "Automatic process group scheduling"
 	select EVENTFD
-- 
1.7.5.4

[PATCH review 16/16] userns: Remove the EXPERMINTAL kconfig tag

[Eric W. Biederman]

From: "Eric W. Biederman" <ebiederm@xmission.com>

While there is more work to be done in the form of allowing
more to happen when you have root inside of a user namespace
there is nothing experimental about them.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 init/Kconfig |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index c22e1b9..f2b1419 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1026,8 +1026,7 @@ config IPC_NS
 	  different IPC objects in different namespaces.
 
 config USER_NS
-	bool "User namespace (EXPERIMENTAL)"
-	depends on EXPERIMENTAL
+	bool "User namespace"
 	default n
 	help
 	  This allows containers, i.e. vservers, to use user namespaces
-- 
1.7.5.4