diff mbox

xfs: fix inode uid/gid initialization

Message ID 20170213194337.GA9852@infradead.org (mailing list archive)
State Superseded
Headers show

Commit Message

Christoph Hellwig Feb. 13, 2017, 7:43 p.m. UTC 
On Mon, Feb 13, 2017 at 09:46:41AM -0800, James Bottomley wrote:
> I was debugging a creation failure using a vfs shifting patch set and
> discovered that xfs itself doesn't actually respect the superblock
> namespace in a couple of places (these showed up as files with the
> wrong ownership in my tests).

Can you submit your test case to xfstests?  I would be good to have
testing for this in the regular test runs.

> The fix is to convert xfs away from hand
> rolling inode_init_owner() and to use the i_uid/gid_read/write
> functions.

What about the various quota users of xfs_kuid_to_uid/gid in
the create / symlink path?  I suspect they should be handle the same.

Also with your patch the di_uid/gid fields should probably just
go away as they are pointless now.  Something like the patch below,
although it still doesn't take care of the quota issues pointed out
above.

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

James Bottomley Feb. 13, 2017, 8:33 p.m. UTC | #1 
On Mon, 2017-02-13 at 11:43 -0800, Christoph Hellwig wrote:
> On Mon, Feb 13, 2017 at 09:46:41AM -0800, James Bottomley wrote:
> > I was debugging a creation failure using a vfs shifting patch set
> > and
> > discovered that xfs itself doesn't actually respect the superblock
> > namespace in a couple of places (these showed up as files with the
> > wrong ownership in my tests).
> 
> Can you submit your test case to xfstests?  I would be good to have
> testing for this in the regular test runs.

I will eventually ... I'm planning on adding a whole set.  This issue
was just found by untarring a container image and then finding the ids
were wrong ... 

> > The fix is to convert xfs away from hand
> > rolling inode_init_owner() and to use the i_uid/gid_read/write
> > functions.
> 
> What about the various quota users of xfs_kuid_to_uid/gid in
> the create / symlink path?

Yes, looking at it again, xfs_qm_vop_dqalloc() is in terms of the
filesystem view, so current_fsuid(), which gives the uid in the kernel
view, needs to be transformed through the s_user_ns to get it into that
view.

Probably there needs to be an inode_fsuid/fsgid() (similar to i_uid/gid
_read())that returns the filesystem view of fsuid/fsgid

>   I suspect they should be handle the same.
> 
> Also with your patch the di_uid/gid fields should probably just
> go away as they are pointless now.  Something like the patch below,
> although it still doesn't take care of the quota issues pointed out
> above.

Yes, I'll go for that.

James

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dave Chinner Feb. 13, 2017, 9:34 p.m. UTC | #2 
On Mon, Feb 13, 2017 at 11:43:37AM -0800, Christoph Hellwig wrote:
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -814,18 +814,10 @@ xfs_ialloc(
>  	if (ip->i_d.di_version == 1)
>  		ip->i_d.di_version = 2;
>  
> -	inode->i_mode = mode;
> +	inode_init_owner(inode, pip ? VFS_I(pip) : NULL, mode);
>  	set_nlink(inode, nlink);
> -	ip->i_d.di_uid = xfs_kuid_to_uid(current_fsuid());
> -	ip->i_d.di_gid = xfs_kgid_to_gid(current_fsgid());
>  	xfs_set_projid(ip, prid);
>  
> -	if (pip && XFS_INHERIT_GID(pip)) {
> -		ip->i_d.di_gid = pip->i_d.di_gid;
> -		if ((VFS_I(pip)->i_mode & S_ISGID) && S_ISDIR(mode))
> -			inode->i_mode |= S_ISGID;
> -	}
> -

Doesn't this hunk break the "nogrpid" mount option?

Cheers,

Dave.
Christoph Hellwig Feb. 14, 2017, 6:08 a.m. UTC | #3 
On Tue, Feb 14, 2017 at 08:34:16AM +1100, Dave Chinner wrote:
> >  
> > -	if (pip && XFS_INHERIT_GID(pip)) {
> > -		ip->i_d.di_gid = pip->i_d.di_gid;
> > -		if ((VFS_I(pip)->i_mode & S_ISGID) && S_ISDIR(mode))
> > -			inode->i_mode |= S_ISGID;
> > -	}
> > -
> 
> Doesn't this hunk break the "nogrpid" mount option?

It does.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
James Bottomley Feb. 14, 2017, 6:27 a.m. UTC | #4 
On Mon, 2017-02-13 at 22:08 -0800, Christoph Hellwig wrote:
> On Tue, Feb 14, 2017 at 08:34:16AM +1100, Dave Chinner wrote:
> > >  
> > > -	if (pip && XFS_INHERIT_GID(pip)) {
> > > -		ip->i_d.di_gid = pip->i_d.di_gid;
> > > -		if ((VFS_I(pip)->i_mode & S_ISGID) &&
> > > S_ISDIR(mode))
> > > -			inode->i_mode |= S_ISGID;
> > > -	}
> > > -
> > 
> > Doesn't this hunk break the "nogrpid" mount option?
> 
> It does.

OK, so I'll fix up the s_user_mount problems and I'll let you sort out
the internals of removing the di_uid/gid if you wish to.  I checked the
quota code and I think there are only a couple of places you're using
the kernel view of the ids where you should be using the filesystem
view.  They're all identified by current_fsuid/fsgid(), so I think
(with the helper in patch 1) that this is the fix.

James

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig Feb. 14, 2017, 7:58 a.m. UTC | #5 
On Mon, Feb 13, 2017 at 10:27:31PM -0800, James Bottomley wrote:
> OK, so I'll fix up the s_user_mount problems and I'll let you sort out
> the internals of removing the di_uid/gid if you wish to.

We'll need to sort that out, and I'd rather do that than reinterpreting
the fields.  But I can take care of it by ammending or redoing your
patch if you don't feel like poking to deep into XFS.

What we need now is test cases exercising a non-standard s_user_ns
so that we have test coverage for these changes.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index d93f9d918cfc..dfe9b02a62bd 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -233,8 +233,8 @@  xfs_inode_from_disk(
 	}
 
 	to->di_format = from->di_format;
-	to->di_uid = be32_to_cpu(from->di_uid);
-	to->di_gid = be32_to_cpu(from->di_gid);
+	i_uid_write(inode, be32_to_cpu(from->di_uid));
+	i_gid_write(inode, be32_to_cpu(from->di_gid));
 	to->di_flushiter = be16_to_cpu(from->di_flushiter);
 
 	/*
@@ -286,8 +286,8 @@  xfs_inode_to_disk(
 
 	to->di_version = from->di_version;
 	to->di_format = from->di_format;
-	to->di_uid = cpu_to_be32(from->di_uid);
-	to->di_gid = cpu_to_be32(from->di_gid);
+	to->di_uid = cpu_to_be32(i_uid_read(inode));
+	to->di_gid = cpu_to_be32(i_gid_read(inode));
 	to->di_projid_lo = cpu_to_be16(from->di_projid_lo);
 	to->di_projid_hi = cpu_to_be16(from->di_projid_hi);
 
diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
index 6848a0afbce7..a4c5502351c4 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.h
+++ b/fs/xfs/libxfs/xfs_inode_buf.h
@@ -31,8 +31,6 @@  struct xfs_icdinode {
 	__int8_t	di_version;	/* inode version */
 	__int8_t	di_format;	/* format of di_c data */
 	__uint16_t	di_flushiter;	/* incremented on flush */
-	__uint32_t	di_uid;		/* owner's user id */
-	__uint32_t	di_gid;		/* owner's group id */
 	__uint16_t	di_projid_lo;	/* lower part of owner's project id */
 	__uint16_t	di_projid_hi;	/* higher part of owner's project id */
 	xfs_fsize_t	di_size;	/* number of bytes in file */
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index de32f0fe47c8..3b0b09a6b15d 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -814,18 +814,10 @@  xfs_ialloc(
 	if (ip->i_d.di_version == 1)
 		ip->i_d.di_version = 2;
 
-	inode->i_mode = mode;
+	inode_init_owner(inode, pip ? VFS_I(pip) : NULL, mode);
 	set_nlink(inode, nlink);
-	ip->i_d.di_uid = xfs_kuid_to_uid(current_fsuid());
-	ip->i_d.di_gid = xfs_kgid_to_gid(current_fsgid());
 	xfs_set_projid(ip, prid);
 
-	if (pip && XFS_INHERIT_GID(pip)) {
-		ip->i_d.di_gid = pip->i_d.di_gid;
-		if ((VFS_I(pip)->i_mode & S_ISGID) && S_ISDIR(mode))
-			inode->i_mode |= S_ISGID;
-	}
-
 	/*
 	 * If the group ID of the new file does not match the effective group
 	 * ID or one of the supplementary group IDs, the S_ISGID bit is cleared
@@ -833,7 +825,7 @@  xfs_ialloc(
 	 */
 	if ((irix_sgid_inherit) &&
 	    (inode->i_mode & S_ISGID) &&
-	    (!in_group_p(xfs_gid_to_kgid(ip->i_d.di_gid))))
+	    (!in_group_p(inode->i_gid)))
 		inode->i_mode &= ~S_ISGID;
 
 	ip->i_d.di_size = 0;
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index d90e7811ccdd..ed9529e3babf 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -335,8 +335,8 @@  xfs_inode_to_log_dinode(
 
 	to->di_version = from->di_version;
 	to->di_format = from->di_format;
-	to->di_uid = from->di_uid;
-	to->di_gid = from->di_gid;
+	to->di_uid = i_uid_read(inode);
+	to->di_gid = i_gid_read(inode);
 	to->di_projid_lo = from->di_projid_lo;
 	to->di_projid_hi = from->di_projid_hi;
 
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index c67cfb451fd3..92bb1317536a 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1333,8 +1333,8 @@  xfs_ioctl_setattr(
 	 * because the i_*dquot fields will get updated anyway.
 	 */
 	if (XFS_IS_QUOTA_ON(mp)) {
-		code = xfs_qm_vop_dqalloc(ip, ip->i_d.di_uid,
-					 ip->i_d.di_gid, fa->fsx_projid,
+		code = xfs_qm_vop_dqalloc(ip, i_uid_read(VFS_I(ip)),
+					 i_gid_read(VFS_I(ip)), fa->fsx_projid,
 					 XFS_QMOPT_PQUOTA, &udqp, NULL, &pdqp);
 		if (code)
 			return code;
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 22c16155f1b4..c3807a7ae466 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -643,8 +643,8 @@  xfs_setattr_nonsize(
 		 */
 		ASSERT(udqp == NULL);
 		ASSERT(gdqp == NULL);
-		error = xfs_qm_vop_dqalloc(ip, xfs_kuid_to_uid(uid),
-					   xfs_kgid_to_gid(gid),
+		error = xfs_qm_vop_dqalloc(ip, from_kuid(inode->i_sb->s_user_ns, uid),
+					   from_kgid(inode->i_sb->s_user_ns, gid),
 					   xfs_get_projid(ip),
 					   qflags, &udqp, &gdqp, NULL);
 		if (error)
@@ -714,8 +714,8 @@  xfs_setattr_nonsize(
 				olddquot1 = xfs_qm_vop_chown(tp, ip,
 							&ip->i_udquot, udqp);
 			}
-			ip->i_d.di_uid = xfs_kuid_to_uid(uid);
 			inode->i_uid = uid;
+
 		}
 		if (!gid_eq(igid, gid)) {
 			if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_GQUOTA_ON(mp)) {
@@ -726,7 +726,6 @@  xfs_setattr_nonsize(
 				olddquot2 = xfs_qm_vop_chown(tp, ip,
 							&ip->i_gdquot, gdqp);
 			}
-			ip->i_d.di_gid = xfs_kgid_to_gid(gid);
 			inode->i_gid = gid;
 		}
 	}
@@ -1213,9 +1212,6 @@  xfs_setup_inode(
 	/* make the inode look hashed for the writeback code */
 	hlist_add_fake(&inode->i_hash);
 
-	inode->i_uid    = xfs_uid_to_kuid(ip->i_d.di_uid);
-	inode->i_gid    = xfs_gid_to_kgid(ip->i_d.di_gid);
-
 	switch (inode->i_mode & S_IFMT) {
 	case S_IFBLK:
 	case S_IFCHR:
diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index 66e881790c17..bcfc7f38e8e2 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -88,8 +88,8 @@  xfs_bulkstat_one_int(
 	buf->bs_projid_lo = dic->di_projid_lo;
 	buf->bs_projid_hi = dic->di_projid_hi;
 	buf->bs_ino = ino;
-	buf->bs_uid = dic->di_uid;
-	buf->bs_gid = dic->di_gid;
+	buf->bs_uid = i_uid_read(inode);
+	buf->bs_gid = i_gid_read(inode);
 	buf->bs_size = dic->di_size;
 
 	buf->bs_nlink = inode->i_nlink;
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index b669b123287b..0f3692123267 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -341,18 +341,18 @@  xfs_qm_dqattach_locked(
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 
 	if (XFS_IS_UQUOTA_ON(mp) && !ip->i_udquot) {
-		error = xfs_qm_dqattach_one(ip, ip->i_d.di_uid, XFS_DQ_USER,
-						flags & XFS_QMOPT_DQALLOC,
-						&ip->i_udquot);
+		error = xfs_qm_dqattach_one(ip, i_uid_read(VFS_I(ip)),
+				XFS_DQ_USER, flags & XFS_QMOPT_DQALLOC,
+				&ip->i_udquot);
 		if (error)
 			goto done;
 		ASSERT(ip->i_udquot);
 	}
 
 	if (XFS_IS_GQUOTA_ON(mp) && !ip->i_gdquot) {
-		error = xfs_qm_dqattach_one(ip, ip->i_d.di_gid, XFS_DQ_GROUP,
-						flags & XFS_QMOPT_DQALLOC,
-						&ip->i_gdquot);
+		error = xfs_qm_dqattach_one(ip, i_gid_read(VFS_I(ip)),
+				XFS_DQ_GROUP, flags & XFS_QMOPT_DQALLOC,
+				&ip->i_gdquot);
 		if (error)
 			goto done;
 		ASSERT(ip->i_gdquot);
@@ -1210,14 +1210,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, i_uid_read(VFS_I(ip)),
 						   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, i_gid_read(VFS_I(ip)),
 						   XFS_DQ_GROUP, nblks, rtblks);
 		if (error)
 			goto error0;
@@ -1650,7 +1650,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 = i_gid_read(VFS_I(ip));
 
 	/*
 	 * Attach the dquot(s) to this inode, doing a dquot allocation
@@ -1665,7 +1665,7 @@  xfs_qm_vop_dqalloc(
 	}
 
 	if ((flags & XFS_QMOPT_UQUOTA) && XFS_IS_UQUOTA_ON(mp)) {
-		if (ip->i_d.di_uid != uid) {
+		if (i_uid_read(VFS_I(ip)) != uid) {
 			/*
 			 * What we need is the dquot that has this uid, and
 			 * if we send the inode to dqget, the uid of the inode
@@ -1701,7 +1701,7 @@  xfs_qm_vop_dqalloc(
 		}
 	}
 	if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
-		if (ip->i_d.di_gid != gid) {
+		if (i_gid_read(VFS_I(ip)) != gid) {
 			xfs_iunlock(ip, lockflags);
 			error = xfs_qm_dqget(mp, NULL, gid,
 						 XFS_DQ_GROUP,
@@ -1835,7 +1835,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 != be32_to_cpu(udqp->q_core.d_id)) {
+	    i_uid_read(VFS_I(ip)) != be32_to_cpu(udqp->q_core.d_id)) {
 		udq_delblks = udqp;
 		/*
 		 * If there are delayed allocation blocks, then we have to
@@ -1848,7 +1848,7 @@  xfs_qm_vop_chown_reserve(
 		}
 	}
 	if (XFS_IS_GQUOTA_ON(ip->i_mount) && gdqp &&
-	    ip->i_d.di_gid != be32_to_cpu(gdqp->q_core.d_id)) {
+	    i_gid_read(VFS_I(ip)) != be32_to_cpu(gdqp->q_core.d_id)) {
 		gdq_delblks = gdqp;
 		if (delblks) {
 			ASSERT(ip->i_gdquot);
@@ -1945,14 +1945,14 @@  xfs_qm_vop_create_dqattach(
 
 	if (udqp && XFS_IS_UQUOTA_ON(mp)) {
 		ASSERT(ip->i_udquot == NULL);
-		ASSERT(ip->i_d.di_uid == be32_to_cpu(udqp->q_core.d_id));
+		ASSERT(i_uid_read(VFS_I(ip)) == 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);
 	}
 	if (gdqp && XFS_IS_GQUOTA_ON(mp)) {
 		ASSERT(ip->i_gdquot == NULL);
-		ASSERT(ip->i_d.di_gid == be32_to_cpu(gdqp->q_core.d_id));
+		ASSERT(i_uid_read(VFS_I(ip)) == 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);
 	}