On Mon, Mar 01, 2021 at 12:05:20PM -0800, Darrick J. Wong wrote:

On Thu, Jan 21, 2021 at 02:19:58PM +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 the relevant helpers.

Note that full-filesystem bulkstat is not supported from inside idmapped
mounts as it is an administrative operation that acts on the whole file
system. The limitation is not applied to the bulkstat single operation
that just operates on a single inode.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Christian Brauner <redacted>
---
/* v2 */

/* v3 */

/* v4 */

/* v5 */
base-commit: 7c53f6b671f4aba70ff15e1b05148b10d58c2837

/* v6 */
unchanged
base-commit: 19c329f6808995b142b3966301f217c831e7cf31
---
 fs/xfs/xfs_acl.c     |  3 +--
 fs/xfs/xfs_file.c    |  4 +++-
 fs/xfs/xfs_inode.c   | 26 +++++++++++++++--------
 fs/xfs/xfs_inode.h   | 16 +++++++++------
 fs/xfs/xfs_ioctl.c   | 35 ++++++++++++++++++-------------
 fs/xfs/xfs_ioctl32.c |  6 ++++--
 fs/xfs/xfs_iops.c    | 49 +++++++++++++++++++++++++-------------------
 fs/xfs/xfs_iops.h    |  3 ++-
 fs/xfs/xfs_itable.c  | 17 +++++++++++----
 fs/xfs/xfs_itable.h  |  1 +
 fs/xfs/xfs_qm.c      |  3 ++-
 fs/xfs/xfs_super.c   |  2 +-
 fs/xfs/xfs_symlink.c |  5 +++--
 fs/xfs/xfs_symlink.h |  5 +++--
 14 files changed, 110 insertions(+), 65 deletions(-)

<snip> Sorry for not noticing until after this went upstream, but...

No problem at all.

quoted

diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index 16ca97a7ff00..ca310a125d1e 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c

@@ -54,10 +54,12 @@ struct xfs_bstat_chunk {
 STATIC int
 xfs_bulkstat_one_int(
 	struct xfs_mount	*mp,
+	struct user_namespace	*mnt_userns,
 	struct xfs_trans	*tp,
 	xfs_ino_t		ino,
 	struct xfs_bstat_chunk	*bc)
 {
+	struct user_namespace	*sb_userns = mp->m_super->s_user_ns;
 	struct xfs_icdinode	*dic;		/* dinode core info pointer */
 	struct xfs_inode	*ip;		/* incore inode pointer */
 	struct inode		*inode;

@@ -86,8 +88,8 @@ xfs_bulkstat_one_int(
 	 */
 	buf->bs_projectid = ip->i_d.di_projid;
 	buf->bs_ino = ino;
-	buf->bs_uid = i_uid_read(inode);
-	buf->bs_gid = i_gid_read(inode);
+	buf->bs_uid = from_kuid(sb_userns, i_uid_into_mnt(mnt_userns, inode));
+	buf->bs_gid = from_kgid(sb_userns, i_gid_into_mnt(mnt_userns, inode));
 	buf->bs_size = dic->di_size;
 
 	buf->bs_nlink = inode->i_nlink;

@@ -173,7 +175,8 @@ xfs_bulkstat_one(
 	if (!bc.buf)
 		return -ENOMEM;
 
-	error = xfs_bulkstat_one_int(breq->mp, NULL, breq->startino, &bc);
+	error = xfs_bulkstat_one_int(breq->mp, breq->mnt_userns, NULL,
+				     breq->startino, &bc);
 
 	kmem_free(bc.buf);
 

@@ -194,9 +197,10 @@ xfs_bulkstat_iwalk(
 	xfs_ino_t		ino,
 	void			*data)
 {
+	struct xfs_bstat_chunk	*bc = data;
 	int			error;
 
-	error = xfs_bulkstat_one_int(mp, tp, ino, data);
+	error = xfs_bulkstat_one_int(mp, bc->breq->mnt_userns, tp, ino, data);
 	/* bulkstat just skips over missing inodes */
 	if (error == -ENOENT || error == -EINVAL)
 		return 0;

@@ -239,6 +243,11 @@ xfs_bulkstat(
 	};
 	int			error;
 
+	if (breq->mnt_userns != &init_user_ns) {
+		xfs_warn_ratelimited(breq->mp,
+			"bulkstat not supported inside of idmapped mounts.");
+		return -EINVAL;

Shouldn't this be -EPERM?

Or -EOPNOTSUPP?

EOPNOTSUPP seems a good choice. Whether or not it's better than EINVAL I
don't know. With my userspace maintainer hat on I would probably say
that EOPNOTSUPP feels a bit more natural and might have the advantage
that it is less overloaded then EINVAL.

Also, I'm not sure why bulkstat won't work in an idmapped mount but
bulkstat_single does?  You can use the singleton version to stat inodes
that aren't inside the submount.

Christoph will very likely have a better informed opinion than I have
but as long as bulkstat is able to discern inodes that need to be
reported idmapped and inodes that don't then I see no reason why this
shouldn't work (at least for privileged users on the host which I think
is the case already).

In any case these changes, if any, aren't vfs changes and so you can
just take them as bugfixes through the xfs tree anyway. So no harm done
in you not spotting it earlier. :)

Thanks for taking another look!
Christian