[PATCH] ntfs: fix acl handling

[PATCH] ntfs: fix acl handling

[Christian Brauner]

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@paragon-software.com>
Cc: ntfs3@lists.linux.dev
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 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

Re: [PATCH] ntfs: fix acl handling

[Christian Brauner]

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@paragon-software.com>
> Cc: ntfs3@lists.linux.dev
> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> ---

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
> 

Re: [PATCH] ntfs: fix acl handling

[Konstantin Komarov]

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@paragon-software.com>
>> Cc: ntfs3@lists.linux.dev
>> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
>> ---
> 
> 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@paragon-software.com>

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

Re: [PATCH] ntfs: fix acl handling

[Christian Brauner]

On Mon, Aug 22, 2022 at 08:40:32PM +0300, Konstantin Komarov wrote:
> 
> 
> 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@paragon-software.com>
> > > Cc: ntfs3@lists.linux.dev
> > > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> > > ---
> > 
> > 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?

Thanks for your reply! I sent this through my tree.
Thanks!
Christian