On Fri, Jan 15, 2021 at 07:51:54AM +1100, Dave Chinner wrote:

On Tue, Jan 12, 2021 at 11:01:19PM +0100, Christian Brauner wrote:

quoted

From: Christoph Hellwig <hch@lst.de>

Enable idmapped mounts for xfs. This basically just means passing down
the user_namespace argument from the VFS methods down to where it is
passed to helper.

Signed-off-by: Christoph Hellwig <hch@lst.de>

....

quoted

@@ -654,6 +658,7 @@ xfs_vn_change_ok(
  */
 static int
 xfs_setattr_nonsize(
+	struct user_namespace	*mnt_userns,
 	struct xfs_inode	*ip,
 	struct iattr		*iattr)
 {

@@ -813,7 +818,7 @@ xfs_setattr_nonsize(
 	 * 	     Posix ACL code seems to care about this issue either.
 	 */
 	if (mask & ATTR_MODE) {
-		error = posix_acl_chmod(&init_user_ns, inode, inode->i_mode);
+		error = posix_acl_chmod(mnt_userns, inode, inode->i_mode);
 		if (error)
 			return error;
 	}

@@ -868,7 +873,7 @@ xfs_setattr_size(
 		 * Use the regular setattr path to update the timestamps.
 		 */
 		iattr->ia_valid &= ~ATTR_SIZE;
-		return xfs_setattr_nonsize(ip, iattr);
+		return xfs_setattr_nonsize(&init_user_ns, ip, iattr);

Shouldn't that be passing mnt_userns?

Hey Dave,

Thanks for taking a look.

This is the time updating codepath.

xfs_setattr_size();
-> xfs_setattr_nonsize(&init_user_ns);

The xfs_setattr_size() helper will assert:

ASSERT((iattr->ia_valid & (ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
	ATTR_MTIME_SET|ATTR_KILL_PRIV|ATTR_TIMES_SET)) == 0);

While the

xfs_setattr_nonsize() helper will further assert:

ASSERT((mask & ATTR_SIZE) == 0);

so xfs_setattr_nonsize() in this callpath is only used to update the

if (!(iattr->ia_valid & (ATTR_CTIME|ATTR_MTIME)))
	return 0;

so there's no interactions with idmappings in any way. Simply passing
mnt_userns might be clearer though.

But if this would be using the wrong idmapping the xfstest suite I added
would've immediately caught that and failed.

But this specific codepath can also be reliably hit from userspace by
doing ftruncate(fd, 0) so just to be extra sure I added truncate tests
to the xfstests now for both the idmapped and non-idmapped case.

Christian