Re: [PATCH 0/5] fs: allow changing idmappings

[Seth Forshee <sforshee@kernel.org>]

On Tue, Jan 28, 2025 at 11:33:38AM +0100, Christian Brauner wrote:
> /* solution */
> 
> So, to avoid all of these pitfalls creating an idmapped mount from an
> already idmapped mount will be done atomically, i.e., a new detached
> mount is created and a new set of mount properties applied to it without
> it ever having been exposed to userspace at all.
> 
> This can be done in two ways. A new flag to open_tree() is added
> OPEN_TREE_CLEAR_IDMAP that clears the old idmapping and returns a mount
> that isn't idmapped. And then it is possible to set mount attributes on
> it again including creation of an idmapped mount.
> 
> This has the consequence that a file descriptor must exist in userspace
> that doesn't have any idmapping applied and it will thus never work in
> unpriviledged scenarios. As a container would be able to remove the
> idmapping of the mount it has been given. That should be avoided.
> 
> Instead, we add open_tree_attr() which works just like open_tree() but
> takes an optional struct mount_attr parameter. This is useful beyond
> idmappings as it fills a gap where a mount never exists in userspace
> without the necessary mount properties applied.
> 
> This is particularly useful for mount options such as
> MOUNT_ATTR_{RDONLY,NOSUID,NODEV,NOEXEC}.
> 
> To create a new idmapped mount the following works:
> 
> // Create a first idmapped mount
> struct mount_attr attr = {
>         .attr_set = MOUNT_ATTR_IDMAP
>         .userns_fd = fd_userns
> };
> 
> fd_tree = open_tree(-EBADF, "/", OPEN_TREE_CLONE, &attr, sizeof(attr));
> move_mount(fd_tree, "", -EBADF, "/mnt", MOVE_MOUNT_F_EMPTY_PATH);
> 
> // Create a second idmapped mount from the first idmapped mount
> attr.attr_set = MOUNT_ATTR_IDMAP;
> attr.userns_fd = fd_userns2;
> fd_tree2 = open_tree(-EBADF, "/mnt", OPEN_TREE_CLONE, &attr, sizeof(attr));
> 
> // Create a second non-idmapped mount from the first idmapped mount:
> memset(&attr, 0, sizeof(attr));
> attr.attr_clr = MOUNT_ATTR_IDMAP;
> fd_tree2 = open_tree(-EBADF, "/mnt", OPEN_TREE_CLONE, &attr, sizeof(attr));

This approach seems reasonable to me, and the patches look good.

Reviewed-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org>