On Wed, Jul 20, 2022 at 02:32:52PM +0200, Christian Brauner wrote:
> While looking at our current POSIX ACL handling in the context of some
> overlayfs work I went through a range of other filesystems checking how they
> handle them currently and encountered ntfs3.
>
> The posic_acl_{from,to}_xattr() helpers always need to operate on the
> filesystem idmapping. Since ntfs3 can only be mounted in the initial user
> namespace the relevant idmapping is init_user_ns.
>
> The posix_acl_{from,to}_xattr() helpers are concerned with translating between
> the kernel internal struct posix_acl{_entry} and the uapi struct
> posix_acl_xattr_{header,entry} and the kernel internal data structure is cached
> filesystem wide.
>
> Additional idmappings such as the caller's idmapping or the mount's idmapping
> are handled higher up in the VFS. Individual filesystems usually do not need to
> concern themselves with these.
>
> The posix_acl_valid() helper is concerned with checking whether the values in
> the kernel internal struct posix_acl can be represented in the filesystem's
> idmapping. IOW, if they can be written to disk. So this helper too needs to
> take the filesystem's idmapping.
>
> Fixes: be71b5cba2e6 ("fs/ntfs3: Add attrib operations")
> Cc: Konstantin Komarov <almaz.alexandrovich@xxxxxxxxxxxxxxxxxxxx>
> Cc: ntfs3@xxxxxxxxxxxxxxx
> Signed-off-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
> ---
Somehow this patch fell through the cracks and this should really be
fixed. Do you plan on sending a PR for this soon or should I just send
it through my tree?
> fs/ntfs3/xattr.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
> index 5e0e0280e70d..3e9118705174 100644
> --- a/fs/ntfs3/xattr.c
> +++ b/fs/ntfs3/xattr.c
> @@ -478,8 +478,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
> }
>
> #ifdef CONFIG_NTFS3_FS_POSIX_ACL
> -static struct posix_acl *ntfs_get_acl_ex(struct user_namespace *mnt_userns,
> - struct inode *inode, int type,
> +static struct posix_acl *ntfs_get_acl_ex(struct inode *inode, int type,
> int locked)
> {
> struct ntfs_inode *ni = ntfs_i(inode);
> @@ -514,7 +513,7 @@ static struct posix_acl *ntfs_get_acl_ex(struct user_namespace *mnt_userns,
>
> /* Translate extended attribute to acl. */
> if (err >= 0) {
> - acl = posix_acl_from_xattr(mnt_userns, buf, err);
> + acl = posix_acl_from_xattr(&init_user_ns, buf, err);
> } else if (err == -ENODATA) {
> acl = NULL;
> } else {
> @@ -537,8 +536,7 @@ struct posix_acl *ntfs_get_acl(struct inode *inode, int type, bool rcu)
> if (rcu)
> return ERR_PTR(-ECHILD);
>
> - /* TODO: init_user_ns? */
> - return ntfs_get_acl_ex(&init_user_ns, inode, type, 0);
> + return ntfs_get_acl_ex(inode, type, 0);
> }
>
> static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
> @@ -595,7 +593,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
> value = kmalloc(size, GFP_NOFS);
> if (!value)
> return -ENOMEM;
> - err = posix_acl_to_xattr(mnt_userns, acl, value, size);
> + err = posix_acl_to_xattr(&init_user_ns, acl, value, size);
> if (err < 0)
> goto out;
> flags = 0;
> @@ -641,7 +639,7 @@ static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns,
> if (!acl)
> return -ENODATA;
>
> - err = posix_acl_to_xattr(mnt_userns, acl, buffer, size);
> + err = posix_acl_to_xattr(&init_user_ns, acl, buffer, size);
> posix_acl_release(acl);
>
> return err;
> @@ -665,12 +663,12 @@ static int ntfs_xattr_set_acl(struct user_namespace *mnt_userns,
> if (!value) {
> acl = NULL;
> } else {
> - acl = posix_acl_from_xattr(mnt_userns, value, size);
> + acl = posix_acl_from_xattr(&init_user_ns, value, size);
> if (IS_ERR(acl))
> return PTR_ERR(acl);
>
> if (acl) {
> - err = posix_acl_valid(mnt_userns, acl);
> + err = posix_acl_valid(&init_user_ns, acl);
> if (err)
> goto release_and_out;
> }
>
> base-commit: ff6992735ade75aae3e35d16b17da1008d753d28
> --
> 2.34.1
>
