Re: [PATCH 3/6] acl: add vfs_set_acl_prepare()

[Seth Forshee <sforshee@digitalocean.com>]

On Mon, Aug 29, 2022 at 02:38:42PM +0200, Christian Brauner wrote:
> Various filesystems store POSIX ACLs on the backing store in their uapi
> format. Such filesystems need to translate from the uapi POSIX ACL
> format into the VFS format during i_op->get_acl(). The VFS provides the
> posix_acl_from_xattr() helper for this task.
> 
> But the usage of posix_acl_from_xattr() is currently ambiguous. It is
> intended to transform from a uapi POSIX ACL  to the VFS represenation.
> For example, when retrieving POSIX ACLs for permission checking during
> lookup or when calling getxattr() to retrieve system.posix_acl_{access,default}.
> 
> Calling posix_acl_from_xattr() during i_op->get_acl() will map the raw
> {g,u}id values stored as ACL_{GROUP,USER} entries in the uapi POSIX ACL
> format into k{g,u}id_t in the filesystem's idmapping and return a struct
> posix_acl ready to be returned to the VFS for caching and to perform
> permission checks on.
> 
> However, posix_acl_from_xattr() is also called during setxattr() for all
> filesystems that rely on VFS provides posix_acl_{access,default}_xattr_handler.
> The posix_acl_xattr_set() handler which is used for the ->set() method
> of posix_acl_{access,default}_xattr_handler uses posix_acl_from_xattr()
> to translate from the uapi POSIX ACL format to the VFS format so that it
> can be passed to the i_op->set_acl() handler of the filesystem or for
> direct caching in case no i_op->set_acl() handler is defined.
> 
> During setxattr() the {g,u}id values stored as ACL_{GROUP,USER} entries
> in the uapi POSIX ACL format aren't raw {g,u}id values that need to be
> mapped according to the filesystem's idmapping. Instead they are {g,u}id
> values in the caller's idmapping which have been generated during
> posix_acl_fix_xattr_from_user(). In other words, they are k{g,u}id_t
> which are passed as raw {g,u}id values abusing the uapi POSIX ACL format
> (Please note that this type safety violation has existed since the
> introduction of k{g,u}id_t. Please see [1] for more details.).
> 
> So when posix_acl_from_xattr() is called in posix_acl_xattr_set() the
> filesystem idmapping is completely irrelevant. Instead, we abuse the
> initial idmapping to recover the k{g,u}id_t base on the value stored in
> raw {g,u}id as ACL_{GROUP,USER} in the uapi POSIX ACL format.
> 
> We need to clearly distinguish betweeen these two operations as it is
> really easy to confuse for filesystems as can be seen in ntfs3.
> 
> In order to do this we factor out make_posix_acl() which takes callbacks
> allowing callers to pass dedicated methods to generate the correct
> k{g,u}id_t. This is just an internal static helper which is not exposed
> to any filesystems but it neatly encapsulates the basic logic of walking
> through a uapi POSIX ACL and returning an allocated VFS POSIX ACL with
> the correct k{g,u}id_t values.
> 
> The posix_acl_from_xattr() helper can then be implemented as a simple
> call to make_posix_acl() with callbacks that generate the correct
> k{g,u}id_t from the raw {g,u}id values in ACL_{GROUP,USER} entries in
> the uapi POSIX ACL format as read from the backing store.
> 
> For setxattr() we add a new helper vfs_set_acl_prepare() which has
> callbacks to map the POSIX ACLs from the uapi format with the k{g,u}id_t
> values stored in raw {g,u}id format in ACL_{GROUP,USER} entries into the
> correct k{g,u}id_t values in the filesystem idmapping. In contrast to
> posix_acl_from_xattr() the vfs_set_acl_prepare() helper needs to take
> the mount idmapping into account. The differences are explained in more
> detail in the kernel doc for the new functions.
> 
> In follow up patches we will remove all abuses of posix_acl_from_xattr()
> for setxattr() operations and replace it with calls to vfs_set_acl_prepare().
> 
> The new vfs_set_acl_prepare() helper allows us to deal with the
> ambiguity in how the POSI ACL uapi struct stores {g,u}id values
> depending on whether this is a getxattr() or setxattr() operation.
> 
> This also allows us to remove the posix_acl_setxattr_idmapped_mnt()
> helper reducing the abuse of the POSIX ACL uapi format to pass values
> that should be distinct types in {g,u}id values stored as
> ACL_{GROUP,USER} entries.
> 
> The removal of posix_acl_setxattr_idmapped_mnt() in turn allows us to
> re-constify the value parameter of vfs_setxattr() which in turn allows
> us to avoid the nasty cast from a const void pointer to a non-const void
> pointer on ovl_do_setxattr().
> 
> Ultimately, the plan is to get rid of the type violations completely and
> never pass the values from k{g,u}id_t as raw {g,u}id in ACL_{GROUP,USER}
> entries in uapi POSIX ACL format. But that's a longer way to go and this
> is a preparatory step.
> 
> Link: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org [1]
> Co-Developed-by: Seth Forshee <sforshee@digitalocean.com>
> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>

I can't really give a Reviewed-by as co-author, but this does lgtm. One
nit below however.

> -/*
> - * Convert from extended attribute to in-memory representation.
> +/**
> + * make_posix_acl - convert POSIX ACLs from uapi to VFS format using the
> + *                  provided callbacks to map ACL_{GROUP,USER} entries into the
> + *                  appropriate format
> + * @mnt_userns: the mount's idmapping
> + * @fs_userns: the filesystem's idmapping
> + * @value: the uapi representation of POSIX ACLs
> + * @size: the size of @void

I think you mean "the size of @value"? This appears in a few other
comments too.