Re: [PATCH v2 0/8] introduce dedicated type for idmapped mounts

[Seth Forshee <sforshee@digitalocean.com>]

On Tue, Jun 21, 2022 at 04:14:46PM +0200, Christian Brauner wrote:
> From: "Christian Brauner (Microsoft)" <brauner@kernel.org>
> 
> Hey everyone,
> 
> /* v2 */
> No major changes. The type got renamed since we agreed that the initial
> name wasn't great. There are some typo fixes in the commit messages and
> a few tweaks to the last commit and added Jan's rvb.
> 
> This series starts to introduce a new vfs{g,u}id_t type. It allows to
> distinguish {g,u}ids on idmapped mounts from filesystem k{g,u}ids.
> 
> We leverage the type framework to increase the safety for filesystems
> and the vfs when dealing with idmapped mounts.
> 
> The series introduces the type and converts the setattr codepaths to
> use the new type and associated helpers.
> 
> Currently these codepaths place the value that will ultimately be
> written to inode->i_{g,u}id into attr->ia_{g,u}id which allows to avoid
> changing a few callsites. But there are drawbacks to this approach.
> 
> As Linus rightly points out it makes some of the permission checks in
> the attribute code harder to understand than they need and should be and
> increases the probability for further issues.
> 
> This series makes it so that the values will always be treated as being
> mapped into the idmapped mount. Only when the filesystem object is
> actually updated will the value be mapped into the filesystem idmapping.
> 
> I first looked into this about ~7 months ago but put it on hold to focus
> on the testsuite. Linus expressed the desire for something like this
> last week so I got back to working on this.
> 
> I'd like to get at least this first series in for v5.20. The conversion
> can the continue until we can remove all the regular non-type safe
> helpers and will only be left with the type safe helpers.
> 
> Thanks!
> Christian

As I mentioned in my other responses I prefer to see comparisons with
invalid ids always evaluate as not equal. You can take or leave that
suggestion, but even without it this looks correct, and I think a
separate type is a good change to avoid confusion. For all the patches,
feel free to add:

Reviewed-by: Seth Forshee <sforshee@digitalocean.com>

Seth