Re: [PATCH 00/25] fs: finish conversion to mnt_idmap

From: Dave Chinner <david@fromorbit.com>
To: Christian Brauner <brauner@kernel.org>
Cc: linux-fsdevel@vger.kernel.org,
	"Seth Forshee (Digital Ocean)" <sforshee@kernel.org>,
	Christoph Hellwig <hch@lst.de>, Al Viro <viro@zeniv.linux.org.uk>
Subject: Re: [PATCH 00/25] fs: finish conversion to mnt_idmap
Date: Wed, 18 Jan 2023 09:31:23 +1100	[thread overview]
Message-ID: <20230117223123.GA937597@dread.disaster.area> (raw)
In-Reply-To: <20230113-fs-idmapped-mnt_idmap-conversion-v1-0-fc84fa7eba67@kernel.org>

On Fri, Jan 13, 2023 at 12:49:09PM +0100, Christian Brauner wrote:
> Hey everyone,
> 
> Last cycle we introduced struct mnt_idmap in
> 256c8aed2b42 ("fs: introduce dedicated idmap type for mounts")
> and converted the posix acl high-level helpers over in
> 5a6f52d20ce3 ("acl: conver higher-level helpers to rely on mnt_idmap").
> 
> This series converts all places that currently still pass around a plain
> namespace attached to a mount to passing around a separate type eliminating
> all bugs that can arise from conflating filesystem and mount idmappings.
> After this series nothing will have changed semantically.
> 
> Currently, functions that map filesystem wide {g,u}ids into a mount
> specific idmapping take two namespace pointers, the pointer to the mount
> idmapping and the pointer to the filesystem idmapping. As they are of the
> same type it is easy to accidently pass a mount idmapping as a filesystem
> idmapping and vica versa. In addition, as the mount idmapping is of the
> same type as the filesystem idmapping, it can be passed to all {g,u}id
> translation functions. This is a source of bugs. We fixed a few such bugs
> already and in fact this series starts with a similar bugfix.
> 
> With the introduction of struct mnt_idmap last cycle we can now eliminate
> all these bugs. Instead of two namespace arguments all functions that map
> filesystem wide {g,u}ids into mount specific idmappings now take a struct
> mnt_idmap and a filesystem namespace argument. This lets the compiler catch
> any error where a mount idmapping is conflated with a filesystem idmapping.
> 
> Similarly, since all functions that generate filesystem wide k{g,u}id_ts
> only accept a namespace as an argument it is impossible to pass a mount
> idmapping to them eliminating the possibility of accidently generating
> nonsense {g,u}ids.
> 
> At the end of this conversion struct mnt_idmap becomes opaque to nearly all
> of the vfs and to all filesystems. It's moved into separate file and this file
> is the only place where it is accessed. In addition to type safety, easier
> maintenance, and easier handling and development for filesystem developers it
> also makes it possible to extend idmappings in the future such that we can
> allow userspace to set up idmapping without having to go through the detour of
> using namespaces at all.
> 
> Note, that this is an additional improvement on top of the introduction of
> the vfs{g,u}id_t conversion we did in earlier cycles which already makes it
> impossible to conflate filesystem wide k{g,u}id_t with mount specific
> vfs{g,u}id_t.
> 
> The series is available in the Git repository at:
> 
> ssh://git@gitolite.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git tags/fs.idmapped.mnt_idmap.conversion.v1
> 
> Fstests, selftests, and LTP pass without regressions.

All the XFS modifications in the series look OK. So for them:

Acked-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com