Re: [PATCH v5 37/42] xfs: support idmapped mounts

From: Christoph Hellwig <hch@infradead.org>
To: Dave Chinner <david@fromorbit.com>
Cc: Christian Brauner <christian.brauner@ubuntu.com>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Christoph Hellwig <hch@infradead.org>,
	linux-fsdevel@vger.kernel.org,
	John Johansen <john.johansen@canonical.com>,
	James Morris <jmorris@namei.org>,
	Mimi Zohar <zohar@linux.ibm.com>,
	Dmitry Kasatkin <dmitry.kasatkin@gmail.com>,
	Stephen Smalley <stephen.smalley.work@gmail.com>,
	Casey Schaufler <casey@schaufler-ca.com>,
	Arnd Bergmann <arnd@arndb.de>,
	Andreas Dilger <adilger.kernel@dilger.ca>,
	OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>,
	Geoffrey Thomas <geofft@ldpreload.com>,
	Mrunal Patel <mpatel@redhat.com>,
	Josh Triplett <josh@joshtriplett.org>,
	Andy Lutomirski <luto@kernel.org>, Theodore Tso <tytso@mit.edu>,
	Alban Crequy <alban@kinvolk.io>, Tycho Andersen <tycho@tycho.ws>,
	David Howells <dhowells@redhat.com>,
	James Bottomley <James.Bottomley@hansenpartnership.com>,
	Seth Forshee <seth.forshee@canonical.com>,
	St?phane Graber <stgraber@ubuntu.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Aleksa Sarai <cyphar@cyphar.com>,
	Lennart Poettering <lennart@poettering.net>,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	smbarber@chromium.org, Phil Estes <estesp@gmail.com>,
	Serge Hallyn <serge@hallyn.com>,
	Kees Cook <keescook@chromium.org>, Todd Kjos <tkjos@google.com>,
	Paul Moore <paul@paul-moore.com>,
	Jonathan Corbet <corbet@lwn.net>,
	containers@lists.linux-foundation.org,
	linux-security-module@vger.kernel.org, linux-api@vger.kernel.org,
	linux-ext4@vger.kernel.org, linux-xfs@vger.kernel.org,
	linux-integrity@vger.kernel.org, selinux@vger.kernel.org,
	Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH v5 37/42] xfs: support idmapped mounts
Date: Fri, 15 Jan 2021 16:22:50 +0000	[thread overview]
Message-ID: <20210115162250.GA2179337@infradead.org> (raw)
In-Reply-To: <20210114205154.GL331610@dread.disaster.area>

On Fri, Jan 15, 2021 at 07:51:54AM +1100, Dave Chinner wrote:
> > @@ -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?

As Christian already explained we an't hit this with anything related
to uids/gids, the only thing that will be updated are the timestamps,
as also mentioned in the comment that only makes it partially into the
diff context.

> >  	trace_xfs_setattr(ip);
> >  
> > -	error = xfs_vn_change_ok(dentry, iattr);
> > +	error = xfs_vn_change_ok(mnt_userns, dentry, iattr);
> >  	if (error)
> >  		return error;
> >  	return xfs_setattr_size(ip, iattr);
> 
> And this passing mnt_userns down into xfs_setattr_size()?  Seems
> like a bit of a landmine...

That being said we could just pass down the argument, even if it doesn't
make much sense for the size update.