Re: [PATCH] ntfs: fix acl handling

[Konstantin Komarov <almaz.alexandrovich@xxxxxxxxxxxxxxxxxxxx>]

On 8/18/22 10:47, Christian Brauner wrote:
> 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?

Thanks for catching this, I've missed this patch.
I've run tests - everything seems to be fine.
I've already sent PR for 6.0 and next PR will probably be sometime in September or later.
Can you send it through your tree?

Signed-off-by: Konstantin Komarov <almaz.alexandrovich@xxxxxxxxxxxxxxxxxxxx>

> >   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