[PATCH 0/3] fs/ntfs3: Refactoring of xattr.c

[PATCH 0/3] fs/ntfs3: Refactoring of xattr.c

[Konstantin Komarov]

Removed function, that already have been in kernel.
Changed locking policy to fix some potential bugs.
Changed code for readability.

Konstantin Komarov (3):
  fs/ntfs3: Use available posix_acl_release instead of
    ntfs_posix_acl_release
  fs/ntfs3: Remove locked argument in ntfs_set_ea
  fs/ntfs3: Refactoring of ntfs_set_ea

 fs/ntfs3/xattr.c | 69 ++++++++++++++++++++++--------------------------
 1 file changed, 32 insertions(+), 37 deletions(-)

-- 
2.33.0

[PATCH 1/3] fs/ntfs3: Use available posix_acl_release instead of ntfs_posix_acl_release

[Konstantin Komarov]

We don't need to maintain ntfs_posix_acl_release.

Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
---
 fs/ntfs3/xattr.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
index 83bbee277e12..253a07d9aa7b 100644
--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c
@@ -475,12 +475,6 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
 }
 
 #ifdef CONFIG_NTFS3_FS_POSIX_ACL
-static inline void ntfs_posix_acl_release(struct posix_acl *acl)
-{
-	if (acl && refcount_dec_and_test(&acl->a_refcount))
-		kfree(acl);
-}
-
 static struct posix_acl *ntfs_get_acl_ex(struct user_namespace *mnt_userns,
 					 struct inode *inode, int type,
 					 int locked)
@@ -641,7 +635,7 @@ static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns,
 		return -ENODATA;
 
 	err = posix_acl_to_xattr(mnt_userns, acl, buffer, size);
-	ntfs_posix_acl_release(acl);
+	posix_acl_release(acl);
 
 	return err;
 }
@@ -678,7 +672,7 @@ static int ntfs_xattr_set_acl(struct user_namespace *mnt_userns,
 	err = ntfs_set_acl(mnt_userns, inode, acl, type);
 
 release_and_out:
-	ntfs_posix_acl_release(acl);
+	posix_acl_release(acl);
 	return err;
 }
 
-- 
2.33.0

[PATCH 2/3] fs/ntfs3: Remove locked argument in ntfs_set_ea

[Konstantin Komarov]

We always need to lock now, because locks became smaller
(see "Move ni_lock_dir and ni_unlock into ntfs_create_inode").

Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
---
 fs/ntfs3/xattr.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
index 253a07d9aa7b..1ab109723b10 100644
--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c
@@ -257,7 +257,7 @@ static int ntfs_get_ea(struct inode *inode, const char *name, size_t name_len,
 
 static noinline int ntfs_set_ea(struct inode *inode, const char *name,
 				size_t name_len, const void *value,
-				size_t val_size, int flags, int locked)
+				size_t val_size, int flags)
 {
 	struct ntfs_inode *ni = ntfs_i(inode);
 	struct ntfs_sb_info *sbi = ni->mi.sbi;
@@ -276,8 +276,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
 	u64 new_sz;
 	void *p;
 
-	if (!locked)
-		ni_lock(ni);
+	ni_lock(ni);
 
 	run_init(&ea_run);
 
@@ -465,8 +464,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
 	mark_inode_dirty(&ni->vfs_inode);
 
 out:
-	if (!locked)
-		ni_unlock(ni);
+	ni_unlock(ni);
 
 	run_close(&ea_run);
 	kfree(ea_all);
@@ -537,7 +535,7 @@ struct posix_acl *ntfs_get_acl(struct inode *inode, int type)
 
 static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
 				    struct inode *inode, struct posix_acl *acl,
-				    int type, int locked)
+				    int type)
 {
 	const char *name;
 	size_t size, name_len;
@@ -594,7 +592,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
 		flags = 0;
 	}
 
-	err = ntfs_set_ea(inode, name, name_len, value, size, flags, locked);
+	err = ntfs_set_ea(inode, name, name_len, value, size, flags);
 	if (err == -ENODATA && !size)
 		err = 0; /* Removing non existed xattr. */
 	if (!err)
@@ -612,7 +610,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
 int ntfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
 		 struct posix_acl *acl, int type)
 {
-	return ntfs_set_acl_ex(mnt_userns, inode, acl, type, 0);
+	return ntfs_set_acl_ex(mnt_userns, inode, acl, type);
 }
 
 static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns,
@@ -693,7 +691,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode,
 
 	if (default_acl) {
 		err = ntfs_set_acl_ex(mnt_userns, inode, default_acl,
-				      ACL_TYPE_DEFAULT, 1);
+				      ACL_TYPE_DEFAULT);
 		posix_acl_release(default_acl);
 	} else {
 		inode->i_default_acl = NULL;
@@ -704,7 +702,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode,
 	else {
 		if (!err)
 			err = ntfs_set_acl_ex(mnt_userns, inode, acl,
-					      ACL_TYPE_ACCESS, 1);
+					      ACL_TYPE_ACCESS);
 		posix_acl_release(acl);
 	}
 
@@ -988,7 +986,7 @@ static noinline int ntfs_setxattr(const struct xattr_handler *handler,
 	}
 #endif
 	/* Deal with NTFS extended attribute. */
-	err = ntfs_set_ea(inode, name, name_len, value, size, flags, 0);
+	err = ntfs_set_ea(inode, name, name_len, value, size, flags);
 
 out:
 	return err;
@@ -1006,26 +1004,26 @@ int ntfs_save_wsl_perm(struct inode *inode)
 
 	value = cpu_to_le32(i_uid_read(inode));
 	err = ntfs_set_ea(inode, "$LXUID", sizeof("$LXUID") - 1, &value,
-			  sizeof(value), 0, 0);
+			  sizeof(value), 0);
 	if (err)
 		goto out;
 
 	value = cpu_to_le32(i_gid_read(inode));
 	err = ntfs_set_ea(inode, "$LXGID", sizeof("$LXGID") - 1, &value,
-			  sizeof(value), 0, 0);
+			  sizeof(value), 0);
 	if (err)
 		goto out;
 
 	value = cpu_to_le32(inode->i_mode);
 	err = ntfs_set_ea(inode, "$LXMOD", sizeof("$LXMOD") - 1, &value,
-			  sizeof(value), 0, 0);
+			  sizeof(value), 0);
 	if (err)
 		goto out;
 
 	if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) {
 		value = cpu_to_le32(inode->i_rdev);
 		err = ntfs_set_ea(inode, "$LXDEV", sizeof("$LXDEV") - 1, &value,
-				  sizeof(value), 0, 0);
+				  sizeof(value), 0);
 		if (err)
 			goto out;
 	}
-- 
2.33.0

[PATCH 3/3] fs/ntfs3: Refactoring of ntfs_set_ea

[Konstantin Komarov]

Make code more readable.
Don't try to read zero bytes.
Add warning when size of exteneded attribute exceeds limit.

Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
---
 fs/ntfs3/xattr.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
index 1ab109723b10..395e71291e28 100644
--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c
@@ -75,6 +75,7 @@ static int ntfs_read_ea(struct ntfs_inode *ni, struct EA_FULL **ea,
 			size_t add_bytes, const struct EA_INFO **info)
 {
 	int err;
+	struct ntfs_sb_info *sbi = ni->mi.sbi;
 	struct ATTR_LIST_ENTRY *le = NULL;
 	struct ATTRIB *attr_info, *attr_ea;
 	void *ea_p;
@@ -99,10 +100,10 @@ static int ntfs_read_ea(struct ntfs_inode *ni, struct EA_FULL **ea,
 
 	/* Check Ea limit. */
 	size = le32_to_cpu((*info)->size);
-	if (size > ni->mi.sbi->ea_max_size)
+	if (size > sbi->ea_max_size)
 		return -EFBIG;
 
-	if (attr_size(attr_ea) > ni->mi.sbi->ea_max_size)
+	if (attr_size(attr_ea) > sbi->ea_max_size)
 		return -EFBIG;
 
 	/* Allocate memory for packed Ea. */
@@ -110,15 +111,16 @@ static int ntfs_read_ea(struct ntfs_inode *ni, struct EA_FULL **ea,
 	if (!ea_p)
 		return -ENOMEM;
 
-	if (attr_ea->non_res) {
+	if (!size) {
+		;
+	} else if (attr_ea->non_res) {
 		struct runs_tree run;
 
 		run_init(&run);
 
 		err = attr_load_runs(attr_ea, ni, &run, NULL);
 		if (!err)
-			err = ntfs_read_run_nb(ni->mi.sbi, &run, 0, ea_p, size,
-					       NULL);
+			err = ntfs_read_run_nb(sbi, &run, 0, ea_p, size, NULL);
 		run_close(&run);
 
 		if (err)
@@ -366,21 +368,22 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
 	new_ea->name[name_len] = 0;
 	memcpy(new_ea->name + name_len + 1, value, val_size);
 	new_pack = le16_to_cpu(ea_info.size_pack) + packed_ea_size(new_ea);
-
-	/* Should fit into 16 bits. */
-	if (new_pack > 0xffff) {
-		err = -EFBIG; // -EINVAL?
-		goto out;
-	}
 	ea_info.size_pack = cpu_to_le16(new_pack);
-
 	/* New size of ATTR_EA. */
 	size += add;
-	if (size > sbi->ea_max_size) {
+	ea_info.size = cpu_to_le32(size);
+
+	/*
+	 * 1. Check ea_info.size_pack for overflow.
+	 * 2. New attibute size must fit value from $AttrDef
+	 */
+	if (new_pack > 0xffff || size > sbi->ea_max_size) {
+		ntfs_inode_warn(
+			inode,
+			"The size of exteneded attributes must not exceed 64K");
 		err = -EFBIG; // -EINVAL?
 		goto out;
 	}
-	ea_info.size = cpu_to_le32(size);
 
 update_ea:
 
-- 
2.33.0

Re: [PATCH 3/3] fs/ntfs3: Refactoring of ntfs_set_ea

[Joe Perches]

On Fri, 2021-09-24 at 19:16 +0300, Konstantin Komarov wrote:
> Make code more readable.
> Don't try to read zero bytes.
> Add warning when size of exteneded attribute exceeds limit.
[]
> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
[]
> @@ -366,21 +368,22 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
[]
> +	ea_info.size = cpu_to_le32(size);
> +
> +	/*
> +	 * 1. Check ea_info.size_pack for overflow.
> +	 * 2. New attibute size must fit value from $AttrDef
> +	 */
> +	if (new_pack > 0xffff || size > sbi->ea_max_size) {
> +		ntfs_inode_warn(
> +			inode,
> +			"The size of exteneded attributes must not exceed 64K");

trivial typo of extended.  Pedants might suggest KiB.

Re: [PATCH 3/3] fs/ntfs3: Refactoring of ntfs_set_ea

[Joe Perches]

On Fri, 2021-09-24 at 19:16 +0300, Konstantin Komarov wrote:
> Make code more readable.
> Don't try to read zero bytes.
> Add warning when size of exteneded attribute exceeds limit.
[]
> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
[]
> @@ -366,21 +368,22 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
[]
> +	ea_info.size = cpu_to_le32(size);
> +
> +	/*
> +	 * 1. Check ea_info.size_pack for overflow.
> +	 * 2. New attibute size must fit value from $AttrDef
> +	 */
> +	if (new_pack > 0xffff || size > sbi->ea_max_size) {
> +		ntfs_inode_warn(
> +			inode,
> +			"The size of exteneded attributes must not exceed 64K");

trivial typo of extended.  Pedants might suggest KiB.

Re: [PATCH 2/3] fs/ntfs3: Remove locked argument in ntfs_set_ea

[Kari Argillander]

On Fri, Sep 24, 2021 at 07:15:50PM +0300, Konstantin Komarov wrote:
> We always need to lock now, because locks became smaller
> (see "Move ni_lock_dir and ni_unlock into ntfs_create_inode").

So basically this actually fixes that commit?

Fixes: d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode")

Or if you do not use fixes atleast use

d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode")

You can add these to your gitconfig

	[core]
		abbrev = 12
	[pretty]
	        fixes = Fixes: %h (\"%s\")
		fixed = Fixes: %h (\"%s\")

And get this annotation with
	git show --pretty=fixes <sha>

Have some comments below also.

> 
> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
> ---
>  fs/ntfs3/xattr.c | 28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
> index 253a07d9aa7b..1ab109723b10 100644
> --- a/fs/ntfs3/xattr.c
> +++ b/fs/ntfs3/xattr.c
> @@ -257,7 +257,7 @@ static int ntfs_get_ea(struct inode *inode, const char *name, size_t name_len,
>  
>  static noinline int ntfs_set_ea(struct inode *inode, const char *name,
>  				size_t name_len, const void *value,
> -				size_t val_size, int flags, int locked)
> +				size_t val_size, int flags)
>  {
>  	struct ntfs_inode *ni = ntfs_i(inode);
>  	struct ntfs_sb_info *sbi = ni->mi.sbi;
> @@ -276,8 +276,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
>  	u64 new_sz;
>  	void *p;
>  
> -	if (!locked)
> -		ni_lock(ni);
> +	ni_lock(ni);
>  
>  	run_init(&ea_run);
>  
> @@ -465,8 +464,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
>  	mark_inode_dirty(&ni->vfs_inode);
>  
>  out:
> -	if (!locked)
> -		ni_unlock(ni);
> +	ni_unlock(ni);
>  
>  	run_close(&ea_run);
>  	kfree(ea_all);
> @@ -537,7 +535,7 @@ struct posix_acl *ntfs_get_acl(struct inode *inode, int type)
>  
>  static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
>  				    struct inode *inode, struct posix_acl *acl,
> -				    int type, int locked)
> +				    int type)
>  {
>  	const char *name;
>  	size_t size, name_len;
> @@ -594,7 +592,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
>  		flags = 0;
>  	}
>  
> -	err = ntfs_set_ea(inode, name, name_len, value, size, flags, locked);
> +	err = ntfs_set_ea(inode, name, name_len, value, size, flags);
>  	if (err == -ENODATA && !size)
>  		err = 0; /* Removing non existed xattr. */
>  	if (!err)
> @@ -612,7 +610,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
>  int ntfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
>  		 struct posix_acl *acl, int type)
>  {
> -	return ntfs_set_acl_ex(mnt_userns, inode, acl, type, 0);
> +	return ntfs_set_acl_ex(mnt_userns, inode, acl, type);
>  }
>  
>  static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns,
> @@ -693,7 +691,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode,
>  
>  	if (default_acl) {
>  		err = ntfs_set_acl_ex(mnt_userns, inode, default_acl,
> -				      ACL_TYPE_DEFAULT, 1);
> +				      ACL_TYPE_DEFAULT);
>  		posix_acl_release(default_acl);
>  	} else {
>  		inode->i_default_acl = NULL;
> @@ -704,7 +702,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode,
>  	else {
>  		if (!err)
>  			err = ntfs_set_acl_ex(mnt_userns, inode, acl,
> -					      ACL_TYPE_ACCESS, 1);
> +					      ACL_TYPE_ACCESS);
>  		posix_acl_release(acl);
>  	}
>  
> @@ -988,7 +986,7 @@ static noinline int ntfs_setxattr(const struct xattr_handler *handler,
>  	}
>  #endif
>  	/* Deal with NTFS extended attribute. */
> -	err = ntfs_set_ea(inode, name, name_len, value, size, flags, 0);
> +	err = ntfs_set_ea(inode, name, name_len, value, size, flags);
>  
>  out:
>  	return err;
> @@ -1006,26 +1004,26 @@ int ntfs_save_wsl_perm(struct inode *inode)
>  
>  	value = cpu_to_le32(i_uid_read(inode));
>  	err = ntfs_set_ea(inode, "$LXUID", sizeof("$LXUID") - 1, &value,
> -			  sizeof(value), 0, 0);
> +			  sizeof(value), 0);
>  	if (err)
>  		goto out;
>  
>  	value = cpu_to_le32(i_gid_read(inode));
>  	err = ntfs_set_ea(inode, "$LXGID", sizeof("$LXGID") - 1, &value,
> -			  sizeof(value), 0, 0);
> +			  sizeof(value), 0);
>  	if (err)
>  		goto out;
>  
>  	value = cpu_to_le32(inode->i_mode);
>  	err = ntfs_set_ea(inode, "$LXMOD", sizeof("$LXMOD") - 1, &value,
> -			  sizeof(value), 0, 0);
> +			  sizeof(value), 0);
>  	if (err)
>  		goto out;
>  
>  	if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) {
>  		value = cpu_to_le32(inode->i_rdev);
>  		err = ntfs_set_ea(inode, "$LXDEV", sizeof("$LXDEV") - 1, &value,
> -				  sizeof(value), 0, 0);
> +				  sizeof(value), 0);

Is this really that we can lock/unlock same lock 4 times in a row in a
ntfs_set_ea? This does not feel correct. 

  Argillander

>  		if (err)
>  			goto out;
>  	}
> -- 
> 2.33.0
> 
> 

Re: [PATCH 1/3] fs/ntfs3: Use available posix_acl_release instead of ntfs_posix_acl_release

[Kari Argillander]

On Fri, Sep 24, 2021 at 07:14:53PM +0300, Konstantin Komarov wrote:
> We don't need to maintain ntfs_posix_acl_release.
> 
> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>

Reviewed-by: Kari Argillander <kari.argillander@gmail.com>

> ---
>  fs/ntfs3/xattr.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
> index 83bbee277e12..253a07d9aa7b 100644
> --- a/fs/ntfs3/xattr.c
> +++ b/fs/ntfs3/xattr.c
> @@ -475,12 +475,6 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
>  }
>  
>  #ifdef CONFIG_NTFS3_FS_POSIX_ACL
> -static inline void ntfs_posix_acl_release(struct posix_acl *acl)
> -{
> -	if (acl && refcount_dec_and_test(&acl->a_refcount))
> -		kfree(acl);
> -}
> -
>  static struct posix_acl *ntfs_get_acl_ex(struct user_namespace *mnt_userns,
>  					 struct inode *inode, int type,
>  					 int locked)
> @@ -641,7 +635,7 @@ static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns,
>  		return -ENODATA;
>  
>  	err = posix_acl_to_xattr(mnt_userns, acl, buffer, size);
> -	ntfs_posix_acl_release(acl);
> +	posix_acl_release(acl);
>  
>  	return err;
>  }
> @@ -678,7 +672,7 @@ static int ntfs_xattr_set_acl(struct user_namespace *mnt_userns,
>  	err = ntfs_set_acl(mnt_userns, inode, acl, type);
>  
>  release_and_out:
> -	ntfs_posix_acl_release(acl);
> +	posix_acl_release(acl);
>  	return err;
>  }
>  
> -- 
> 2.33.0
> 
> 

Re: [PATCH 2/3] fs/ntfs3: Remove locked argument in ntfs_set_ea

[Konstantin Komarov]

On 25.09.2021 11:49, Kari Argillander wrote:
> On Fri, Sep 24, 2021 at 07:15:50PM +0300, Konstantin Komarov wrote:
>> We always need to lock now, because locks became smaller
>> (see "Move ni_lock_dir and ni_unlock into ntfs_create_inode").
> 
> So basically this actually fixes that commit?
> 
> Fixes: d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode")
> 
> Or if you do not use fixes atleast use
> 
> d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode")
> 
> You can add these to your gitconfig
> 
> 	[core]
> 		abbrev = 12
> 	[pretty]
> 	        fixes = Fixes: %h (\"%s\")
> 		fixed = Fixes: %h (\"%s\")
> 
> And get this annotation with
> 	git show --pretty=fixes <sha>
> 
> Have some comments below also.
> 
>>
>> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
>> ---
>>  fs/ntfs3/xattr.c | 28 +++++++++++++---------------
>>  1 file changed, 13 insertions(+), 15 deletions(-)
>>
>> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
>> index 253a07d9aa7b..1ab109723b10 100644
>> --- a/fs/ntfs3/xattr.c
>> +++ b/fs/ntfs3/xattr.c
>> @@ -257,7 +257,7 @@ static int ntfs_get_ea(struct inode *inode, const char *name, size_t name_len,
>>  
>>  static noinline int ntfs_set_ea(struct inode *inode, const char *name,
>>  				size_t name_len, const void *value,
>> -				size_t val_size, int flags, int locked)
>> +				size_t val_size, int flags)
>>  {
>>  	struct ntfs_inode *ni = ntfs_i(inode);
>>  	struct ntfs_sb_info *sbi = ni->mi.sbi;
>> @@ -276,8 +276,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
>>  	u64 new_sz;
>>  	void *p;
>>  
>> -	if (!locked)
>> -		ni_lock(ni);
>> +	ni_lock(ni);
>>  
>>  	run_init(&ea_run);
>>  
>> @@ -465,8 +464,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
>>  	mark_inode_dirty(&ni->vfs_inode);
>>  
>>  out:
>> -	if (!locked)
>> -		ni_unlock(ni);
>> +	ni_unlock(ni);
>>  
>>  	run_close(&ea_run);
>>  	kfree(ea_all);
>> @@ -537,7 +535,7 @@ struct posix_acl *ntfs_get_acl(struct inode *inode, int type)
>>  
>>  static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
>>  				    struct inode *inode, struct posix_acl *acl,
>> -				    int type, int locked)
>> +				    int type)
>>  {
>>  	const char *name;
>>  	size_t size, name_len;
>> @@ -594,7 +592,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
>>  		flags = 0;
>>  	}
>>  
>> -	err = ntfs_set_ea(inode, name, name_len, value, size, flags, locked);
>> +	err = ntfs_set_ea(inode, name, name_len, value, size, flags);
>>  	if (err == -ENODATA && !size)
>>  		err = 0; /* Removing non existed xattr. */
>>  	if (!err)
>> @@ -612,7 +610,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
>>  int ntfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
>>  		 struct posix_acl *acl, int type)
>>  {
>> -	return ntfs_set_acl_ex(mnt_userns, inode, acl, type, 0);
>> +	return ntfs_set_acl_ex(mnt_userns, inode, acl, type);
>>  }
>>  
>>  static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns,
>> @@ -693,7 +691,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode,
>>  
>>  	if (default_acl) {
>>  		err = ntfs_set_acl_ex(mnt_userns, inode, default_acl,
>> -				      ACL_TYPE_DEFAULT, 1);
>> +				      ACL_TYPE_DEFAULT);
>>  		posix_acl_release(default_acl);
>>  	} else {
>>  		inode->i_default_acl = NULL;
>> @@ -704,7 +702,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode,
>>  	else {
>>  		if (!err)
>>  			err = ntfs_set_acl_ex(mnt_userns, inode, acl,
>> -					      ACL_TYPE_ACCESS, 1);
>> +					      ACL_TYPE_ACCESS);
>>  		posix_acl_release(acl);
>>  	}
>>  
>> @@ -988,7 +986,7 @@ static noinline int ntfs_setxattr(const struct xattr_handler *handler,
>>  	}
>>  #endif
>>  	/* Deal with NTFS extended attribute. */
>> -	err = ntfs_set_ea(inode, name, name_len, value, size, flags, 0);
>> +	err = ntfs_set_ea(inode, name, name_len, value, size, flags);
>>  
>>  out:
>>  	return err;
>> @@ -1006,26 +1004,26 @@ int ntfs_save_wsl_perm(struct inode *inode)
>>  
>>  	value = cpu_to_le32(i_uid_read(inode));
>>  	err = ntfs_set_ea(inode, "$LXUID", sizeof("$LXUID") - 1, &value,
>> -			  sizeof(value), 0, 0);
>> +			  sizeof(value), 0);
>>  	if (err)
>>  		goto out;
>>  
>>  	value = cpu_to_le32(i_gid_read(inode));
>>  	err = ntfs_set_ea(inode, "$LXGID", sizeof("$LXGID") - 1, &value,
>> -			  sizeof(value), 0, 0);
>> +			  sizeof(value), 0);
>>  	if (err)
>>  		goto out;
>>  
>>  	value = cpu_to_le32(inode->i_mode);
>>  	err = ntfs_set_ea(inode, "$LXMOD", sizeof("$LXMOD") - 1, &value,
>> -			  sizeof(value), 0, 0);
>> +			  sizeof(value), 0);
>>  	if (err)
>>  		goto out;
>>  
>>  	if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) {
>>  		value = cpu_to_le32(inode->i_rdev);
>>  		err = ntfs_set_ea(inode, "$LXDEV", sizeof("$LXDEV") - 1, &value,
>> -				  sizeof(value), 0, 0);
>> +				  sizeof(value), 0);
> 
> Is this really that we can lock/unlock same lock 4 times in a row in a
> ntfs_set_ea? This does not feel correct. 
> 
>   Argillander
> 

How it was working before d562e901f25d 
"fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode":

ntfs_create (lock mutex) =>
ntfs_create_inode =>
ntfs_save_wsl_perm (we are under lock here) =>
return to ntfs_create and unlock

How it works with d562e901f25d:

ntfs_create => 
ntfs_create_inode (lock in line 1201 file fs/ntfs3/inode.c 
and unlock in line 1557) => 
ntfs_save_wsl_perm (we aren't under lock here in line 1605)

So we need to lock 4 times because there are 4 ntfs_set_ea calls.
But now there can be done more work between those calls
in other threads, locks became more granular.

>>  		if (err)
>>  			goto out;
>>  	}
>> -- 
>> 2.33.0
>>
>>

Re: [PATCH 2/3] fs/ntfs3: Remove locked argument in ntfs_set_ea

[Kari Argillander]

On Mon, Sep 27, 2021 at 06:10:00PM +0300, Konstantin Komarov wrote:
> 
> 
> On 25.09.2021 11:49, Kari Argillander wrote:
> > On Fri, Sep 24, 2021 at 07:15:50PM +0300, Konstantin Komarov wrote:
> >> We always need to lock now, because locks became smaller
> >> (see "Move ni_lock_dir and ni_unlock into ntfs_create_inode").
> > 
> > So basically this actually fixes that commit?
> > 
> > Fixes: d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode")
> > 
> > Or if you do not use fixes atleast use
> > 
> > d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode")
> > 
> > You can add these to your gitconfig
> > 
> > 	[core]
> > 		abbrev = 12
> > 	[pretty]
> > 	        fixes = Fixes: %h (\"%s\")
> > 		fixed = Fixes: %h (\"%s\")
> > 
> > And get this annotation with
> > 	git show --pretty=fixes <sha>
> > 
> > Have some comments below also.
> > 
> >>
> >> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
> >> ---
> >>  fs/ntfs3/xattr.c | 28 +++++++++++++---------------
> >>  1 file changed, 13 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
> >> index 253a07d9aa7b..1ab109723b10 100644
> >> --- a/fs/ntfs3/xattr.c
> >> +++ b/fs/ntfs3/xattr.c
> >> @@ -257,7 +257,7 @@ static int ntfs_get_ea(struct inode *inode, const char *name, size_t name_len,
> >>  
> >>  static noinline int ntfs_set_ea(struct inode *inode, const char *name,
> >>  				size_t name_len, const void *value,
> >> -				size_t val_size, int flags, int locked)

Maybe we should leave int locked and ...

> >> +				size_t val_size, int flags)
> >>  {
> >>  	struct ntfs_inode *ni = ntfs_i(inode);
> >>  	struct ntfs_sb_info *sbi = ni->mi.sbi;
> >> @@ -276,8 +276,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
> >>  	u64 new_sz;
> >>  	void *p;
> >>  
> >> -	if (!locked)
> >> -		ni_lock(ni);
> >> +	ni_lock(ni);
> >>  
> >>  	run_init(&ea_run);
> >>  
> >> @@ -465,8 +464,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
> >>  	mark_inode_dirty(&ni->vfs_inode);
> >>  
> >>  out:
> >> -	if (!locked)
> >> -		ni_unlock(ni);
> >> +	ni_unlock(ni);
> >>  
> >>  	run_close(&ea_run);
> >>  	kfree(ea_all);
> >> @@ -537,7 +535,7 @@ struct posix_acl *ntfs_get_acl(struct inode *inode, int type)
> >>  
> >>  static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
> >>  				    struct inode *inode, struct posix_acl *acl,
> >> -				    int type, int locked)
> >> +				    int type)
> >>  {
> >>  	const char *name;
> >>  	size_t size, name_len;
> >> @@ -594,7 +592,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
> >>  		flags = 0;
> >>  	}
> >>  
> >> -	err = ntfs_set_ea(inode, name, name_len, value, size, flags, locked);
> >> +	err = ntfs_set_ea(inode, name, name_len, value, size, flags);
> >>  	if (err == -ENODATA && !size)
> >>  		err = 0; /* Removing non existed xattr. */
> >>  	if (!err)
> >> @@ -612,7 +610,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
> >>  int ntfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
> >>  		 struct posix_acl *acl, int type)
> >>  {
> >> -	return ntfs_set_acl_ex(mnt_userns, inode, acl, type, 0);
> >> +	return ntfs_set_acl_ex(mnt_userns, inode, acl, type);
> >>  }
> >>  
> >>  static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns,
> >> @@ -693,7 +691,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode,
> >>  
> >>  	if (default_acl) {
> >>  		err = ntfs_set_acl_ex(mnt_userns, inode, default_acl,
> >> -				      ACL_TYPE_DEFAULT, 1);
> >> +				      ACL_TYPE_DEFAULT);
> >>  		posix_acl_release(default_acl);
> >>  	} else {
> >>  		inode->i_default_acl = NULL;
> >> @@ -704,7 +702,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode,
> >>  	else {
> >>  		if (!err)
> >>  			err = ntfs_set_acl_ex(mnt_userns, inode, acl,
> >> -					      ACL_TYPE_ACCESS, 1);
> >> +					      ACL_TYPE_ACCESS);
> >>  		posix_acl_release(acl);
> >>  	}
> >>  
> >> @@ -988,7 +986,7 @@ static noinline int ntfs_setxattr(const struct xattr_handler *handler,
> >>  	}
> >>  #endif
> >>  	/* Deal with NTFS extended attribute. */
> >> -	err = ntfs_set_ea(inode, name, name_len, value, size, flags, 0);
> >> +	err = ntfs_set_ea(inode, name, name_len, value, size, flags);
> >>  
> >>  out:
> >>  	return err;
> >> @@ -1006,26 +1004,26 @@ int ntfs_save_wsl_perm(struct inode *inode)
> >>  

do lock here and ...

> >>  	value = cpu_to_le32(i_uid_read(inode));
> >>  	err = ntfs_set_ea(inode, "$LXUID", sizeof("$LXUID") - 1, &value,
> >> -			  sizeof(value), 0, 0);
> >> +			  sizeof(value), 0);
> >>  	if (err)
> >>  		goto out;
> >>  
> >>  	value = cpu_to_le32(i_gid_read(inode));
> >>  	err = ntfs_set_ea(inode, "$LXGID", sizeof("$LXGID") - 1, &value,
> >> -			  sizeof(value), 0, 0);
> >> +			  sizeof(value), 0);
> >>  	if (err)
> >>  		goto out;
> >>  
> >>  	value = cpu_to_le32(inode->i_mode);
> >>  	err = ntfs_set_ea(inode, "$LXMOD", sizeof("$LXMOD") - 1, &value,
> >> -			  sizeof(value), 0, 0);
> >> +			  sizeof(value), 0);
> >>  	if (err)
> >>  		goto out;
> >>  
> >>  	if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) {
> >>  		value = cpu_to_le32(inode->i_rdev);
> >>  		err = ntfs_set_ea(inode, "$LXDEV", sizeof("$LXDEV") - 1, &value,
> >> -				  sizeof(value), 0, 0);
> >> +				  sizeof(value), 0);

unlock here. Of course unlock also in error path.

> > 
> > Is this really that we can lock/unlock same lock 4 times in a row in a
> > ntfs_set_ea? This does not feel correct. 
> > 
> >   Argillander
> > 
> 
> How it was working before d562e901f25d 
> "fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode":
> 
> ntfs_create (lock mutex) =>
> ntfs_create_inode =>
> ntfs_save_wsl_perm (we are under lock here) =>
> return to ntfs_create and unlock
> 
> How it works with d562e901f25d:
> 
> ntfs_create => 
> ntfs_create_inode (lock in line 1201 file fs/ntfs3/inode.c 
> and unlock in line 1557) => 
> ntfs_save_wsl_perm (we aren't under lock here in line 1605)
> 
> So we need to lock 4 times because there are 4 ntfs_set_ea calls.
> But now there can be done more work between those calls
> in other threads, locks became more granular.

Yeah but locking and locking 4 times when we can do it just ones is
quite waste. Please consider my suggestion above or tell what is wrong
with it. 

  Argillander

> 
> >>  		if (err)
> >>  			goto out;
> >>  	}
> >> -- 
> >> 2.33.0
> >>
> >>

Re: [PATCH 2/3] fs/ntfs3: Remove locked argument in ntfs_set_ea

[Konstantin Komarov]

On 27.09.2021 22:10, Kari Argillander wrote:
> On Mon, Sep 27, 2021 at 06:10:00PM +0300, Konstantin Komarov wrote:
>>
>>
>> On 25.09.2021 11:49, Kari Argillander wrote:
>>> On Fri, Sep 24, 2021 at 07:15:50PM +0300, Konstantin Komarov wrote:
>>>> We always need to lock now, because locks became smaller
>>>> (see "Move ni_lock_dir and ni_unlock into ntfs_create_inode").
>>>
>>> So basically this actually fixes that commit?
>>>
>>> Fixes: d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode")
>>>
>>> Or if you do not use fixes atleast use
>>>
>>> d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode")
>>>
>>> You can add these to your gitconfig
>>>
>>> 	[core]
>>> 		abbrev = 12
>>> 	[pretty]
>>> 	        fixes = Fixes: %h (\"%s\")
>>> 		fixed = Fixes: %h (\"%s\")
>>>
>>> And get this annotation with
>>> 	git show --pretty=fixes <sha>
>>>
>>> Have some comments below also.
>>>
>>>>
>>>> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
>>>> ---
>>>>  fs/ntfs3/xattr.c | 28 +++++++++++++---------------
>>>>  1 file changed, 13 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
>>>> index 253a07d9aa7b..1ab109723b10 100644
>>>> --- a/fs/ntfs3/xattr.c
>>>> +++ b/fs/ntfs3/xattr.c
>>>> @@ -257,7 +257,7 @@ static int ntfs_get_ea(struct inode *inode, const char *name, size_t name_len,
>>>>  
>>>>  static noinline int ntfs_set_ea(struct inode *inode, const char *name,
>>>>  				size_t name_len, const void *value,
>>>> -				size_t val_size, int flags, int locked)
> 
> Maybe we should leave int locked and ...
> 
>>>> +				size_t val_size, int flags)
>>>>  {
>>>>  	struct ntfs_inode *ni = ntfs_i(inode);
>>>>  	struct ntfs_sb_info *sbi = ni->mi.sbi;
>>>> @@ -276,8 +276,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
>>>>  	u64 new_sz;
>>>>  	void *p;
>>>>  
>>>> -	if (!locked)
>>>> -		ni_lock(ni);
>>>> +	ni_lock(ni);
>>>>  
>>>>  	run_init(&ea_run);
>>>>  
>>>> @@ -465,8 +464,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
>>>>  	mark_inode_dirty(&ni->vfs_inode);
>>>>  
>>>>  out:
>>>> -	if (!locked)
>>>> -		ni_unlock(ni);
>>>> +	ni_unlock(ni);
>>>>  
>>>>  	run_close(&ea_run);
>>>>  	kfree(ea_all);
>>>> @@ -537,7 +535,7 @@ struct posix_acl *ntfs_get_acl(struct inode *inode, int type)
>>>>  
>>>>  static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
>>>>  				    struct inode *inode, struct posix_acl *acl,
>>>> -				    int type, int locked)
>>>> +				    int type)
>>>>  {
>>>>  	const char *name;
>>>>  	size_t size, name_len;
>>>> @@ -594,7 +592,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
>>>>  		flags = 0;
>>>>  	}
>>>>  
>>>> -	err = ntfs_set_ea(inode, name, name_len, value, size, flags, locked);
>>>> +	err = ntfs_set_ea(inode, name, name_len, value, size, flags);
>>>>  	if (err == -ENODATA && !size)
>>>>  		err = 0; /* Removing non existed xattr. */
>>>>  	if (!err)
>>>> @@ -612,7 +610,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
>>>>  int ntfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
>>>>  		 struct posix_acl *acl, int type)
>>>>  {
>>>> -	return ntfs_set_acl_ex(mnt_userns, inode, acl, type, 0);
>>>> +	return ntfs_set_acl_ex(mnt_userns, inode, acl, type);
>>>>  }
>>>>  
>>>>  static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns,
>>>> @@ -693,7 +691,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode,
>>>>  
>>>>  	if (default_acl) {
>>>>  		err = ntfs_set_acl_ex(mnt_userns, inode, default_acl,
>>>> -				      ACL_TYPE_DEFAULT, 1);
>>>> +				      ACL_TYPE_DEFAULT);
>>>>  		posix_acl_release(default_acl);
>>>>  	} else {
>>>>  		inode->i_default_acl = NULL;
>>>> @@ -704,7 +702,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode,
>>>>  	else {
>>>>  		if (!err)
>>>>  			err = ntfs_set_acl_ex(mnt_userns, inode, acl,
>>>> -					      ACL_TYPE_ACCESS, 1);
>>>> +					      ACL_TYPE_ACCESS);
>>>>  		posix_acl_release(acl);
>>>>  	}
>>>>  
>>>> @@ -988,7 +986,7 @@ static noinline int ntfs_setxattr(const struct xattr_handler *handler,
>>>>  	}
>>>>  #endif
>>>>  	/* Deal with NTFS extended attribute. */
>>>> -	err = ntfs_set_ea(inode, name, name_len, value, size, flags, 0);
>>>> +	err = ntfs_set_ea(inode, name, name_len, value, size, flags);
>>>>  
>>>>  out:
>>>>  	return err;
>>>> @@ -1006,26 +1004,26 @@ int ntfs_save_wsl_perm(struct inode *inode)
>>>>  
> 
> do lock here and ...
> 
>>>>  	value = cpu_to_le32(i_uid_read(inode));
>>>>  	err = ntfs_set_ea(inode, "$LXUID", sizeof("$LXUID") - 1, &value,
>>>> -			  sizeof(value), 0, 0);
>>>> +			  sizeof(value), 0);
>>>>  	if (err)
>>>>  		goto out;
>>>>  
>>>>  	value = cpu_to_le32(i_gid_read(inode));
>>>>  	err = ntfs_set_ea(inode, "$LXGID", sizeof("$LXGID") - 1, &value,
>>>> -			  sizeof(value), 0, 0);
>>>> +			  sizeof(value), 0);
>>>>  	if (err)
>>>>  		goto out;
>>>>  
>>>>  	value = cpu_to_le32(inode->i_mode);
>>>>  	err = ntfs_set_ea(inode, "$LXMOD", sizeof("$LXMOD") - 1, &value,
>>>> -			  sizeof(value), 0, 0);
>>>> +			  sizeof(value), 0);
>>>>  	if (err)
>>>>  		goto out;
>>>>  
>>>>  	if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) {
>>>>  		value = cpu_to_le32(inode->i_rdev);
>>>>  		err = ntfs_set_ea(inode, "$LXDEV", sizeof("$LXDEV") - 1, &value,
>>>> -				  sizeof(value), 0, 0);
>>>> +				  sizeof(value), 0);
> 
> unlock here. Of course unlock also in error path.
> 
>>>
>>> Is this really that we can lock/unlock same lock 4 times in a row in a
>>> ntfs_set_ea? This does not feel correct. 
>>>
>>>   Argillander
>>>
>>
>> How it was working before d562e901f25d 
>> "fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode":
>>
>> ntfs_create (lock mutex) =>
>> ntfs_create_inode =>
>> ntfs_save_wsl_perm (we are under lock here) =>
>> return to ntfs_create and unlock
>>
>> How it works with d562e901f25d:
>>
>> ntfs_create => 
>> ntfs_create_inode (lock in line 1201 file fs/ntfs3/inode.c 
>> and unlock in line 1557) => 
>> ntfs_save_wsl_perm (we aren't under lock here in line 1605)
>>
>> So we need to lock 4 times because there are 4 ntfs_set_ea calls.
>> But now there can be done more work between those calls
>> in other threads, locks became more granular.
> 
> Yeah but locking and locking 4 times when we can do it just ones is
> quite waste. Please consider my suggestion above or tell what is wrong
> with it. 
> 
>   Argillander
> 

If I've understood correctly, you want to lock once in start of function 
ntfs_save_wsl_perm and unlock at the end of it.
It takes care of locks for those 4 calls to ntfs_set_ea.

But there are still other calls to ntfs_set_ea, that aren't protected
in this case:
function ntfs_set_acl_ex (line 603 file fs/ntfs3/xattr.c)
err = ntfs_set_ea(inode, name, name_len, value, size, flags, locked);

This function called there:
- function ntfs_set_acl  (line 621 file fs/ntfs3/xattr.c)
return ntfs_set_acl_ex(mnt_userns, inode, acl, type);
- function ntfs_init_acl (line 701 file fs/ntfs3/xattr.c)
err = ntfs_set_acl_ex(mnt_userns, inode, default_acl,
- function ntfs_init_acl (line 712 file fs/ntfs3/xattr.c)
err = ntfs_set_acl_ex(mnt_userns, inode, acl,

So there are many entry points to ntfs_set_ea (and can be added more).
Of course we can let int locked remain for these situations,
but in my opinion it makes code a lot less readable
(that was the reason to start looking into locking).
I'm not sure, that winning some lock/unlocking
in one specific scenario is worth it.

>>
>>>>  		if (err)
>>>>  			goto out;
>>>>  	}
>>>> -- 
>>>> 2.33.0
>>>>
>>>>

Re: [PATCH 2/3] fs/ntfs3: Remove locked argument in ntfs_set_ea

[Kari Argillander]

On Tue, Sep 28, 2021 at 09:01:38PM +0300, Konstantin Komarov wrote:
> 
> 
> On 27.09.2021 22:10, Kari Argillander wrote:
> > On Mon, Sep 27, 2021 at 06:10:00PM +0300, Konstantin Komarov wrote:
> >>
> >>
> >> On 25.09.2021 11:49, Kari Argillander wrote:
> >>> On Fri, Sep 24, 2021 at 07:15:50PM +0300, Konstantin Komarov wrote:
> >>>> We always need to lock now, because locks became smaller
> >>>> (see "Move ni_lock_dir and ni_unlock into ntfs_create_inode").
> >>>
> >>> So basically this actually fixes that commit?
> >>>
> >>> Fixes: d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode")
> >>>
> >>> Or if you do not use fixes atleast use
> >>>
> >>> d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode")
> >>>
> >>> You can add these to your gitconfig
> >>>
> >>> 	[core]
> >>> 		abbrev = 12
> >>> 	[pretty]
> >>> 	        fixes = Fixes: %h (\"%s\")
> >>> 		fixed = Fixes: %h (\"%s\")
> >>>
> >>> And get this annotation with
> >>> 	git show --pretty=fixes <sha>
> >>>
> >>> Have some comments below also.
> >>>
> >>>>
> >>>> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
> >>>> ---
> >>>>  fs/ntfs3/xattr.c | 28 +++++++++++++---------------
> >>>>  1 file changed, 13 insertions(+), 15 deletions(-)
> >>>>
> >>>> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
> >>>> index 253a07d9aa7b..1ab109723b10 100644
> >>>> --- a/fs/ntfs3/xattr.c
> >>>> +++ b/fs/ntfs3/xattr.c
> >>>> @@ -257,7 +257,7 @@ static int ntfs_get_ea(struct inode *inode, const char *name, size_t name_len,
> >>>>  
> >>>>  static noinline int ntfs_set_ea(struct inode *inode, const char *name,
> >>>>  				size_t name_len, const void *value,
> >>>> -				size_t val_size, int flags, int locked)
> > 
> > Maybe we should leave int locked and ...
> > 
> >>>> +				size_t val_size, int flags)
> >>>>  {
> >>>>  	struct ntfs_inode *ni = ntfs_i(inode);
> >>>>  	struct ntfs_sb_info *sbi = ni->mi.sbi;
> >>>> @@ -276,8 +276,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
> >>>>  	u64 new_sz;
> >>>>  	void *p;
> >>>>  
> >>>> -	if (!locked)
> >>>> -		ni_lock(ni);
> >>>> +	ni_lock(ni);
> >>>>  
> >>>>  	run_init(&ea_run);
> >>>>  
> >>>> @@ -465,8 +464,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
> >>>>  	mark_inode_dirty(&ni->vfs_inode);
> >>>>  
> >>>>  out:
> >>>> -	if (!locked)
> >>>> -		ni_unlock(ni);
> >>>> +	ni_unlock(ni);
> >>>>  
> >>>>  	run_close(&ea_run);
> >>>>  	kfree(ea_all);
> >>>> @@ -537,7 +535,7 @@ struct posix_acl *ntfs_get_acl(struct inode *inode, int type)
> >>>>  
> >>>>  static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
> >>>>  				    struct inode *inode, struct posix_acl *acl,
> >>>> -				    int type, int locked)
> >>>> +				    int type)
> >>>>  {
> >>>>  	const char *name;
> >>>>  	size_t size, name_len;
> >>>> @@ -594,7 +592,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
> >>>>  		flags = 0;
> >>>>  	}
> >>>>  
> >>>> -	err = ntfs_set_ea(inode, name, name_len, value, size, flags, locked);
> >>>> +	err = ntfs_set_ea(inode, name, name_len, value, size, flags);
> >>>>  	if (err == -ENODATA && !size)
> >>>>  		err = 0; /* Removing non existed xattr. */
> >>>>  	if (!err)
> >>>> @@ -612,7 +610,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
> >>>>  int ntfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
> >>>>  		 struct posix_acl *acl, int type)
> >>>>  {
> >>>> -	return ntfs_set_acl_ex(mnt_userns, inode, acl, type, 0);
> >>>> +	return ntfs_set_acl_ex(mnt_userns, inode, acl, type);
> >>>>  }
> >>>>  
> >>>>  static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns,
> >>>> @@ -693,7 +691,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode,
> >>>>  
> >>>>  	if (default_acl) {
> >>>>  		err = ntfs_set_acl_ex(mnt_userns, inode, default_acl,
> >>>> -				      ACL_TYPE_DEFAULT, 1);
> >>>> +				      ACL_TYPE_DEFAULT);
> >>>>  		posix_acl_release(default_acl);
> >>>>  	} else {
> >>>>  		inode->i_default_acl = NULL;
> >>>> @@ -704,7 +702,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode,
> >>>>  	else {
> >>>>  		if (!err)
> >>>>  			err = ntfs_set_acl_ex(mnt_userns, inode, acl,
> >>>> -					      ACL_TYPE_ACCESS, 1);
> >>>> +					      ACL_TYPE_ACCESS);
> >>>>  		posix_acl_release(acl);
> >>>>  	}
> >>>>  
> >>>> @@ -988,7 +986,7 @@ static noinline int ntfs_setxattr(const struct xattr_handler *handler,
> >>>>  	}
> >>>>  #endif
> >>>>  	/* Deal with NTFS extended attribute. */
> >>>> -	err = ntfs_set_ea(inode, name, name_len, value, size, flags, 0);
> >>>> +	err = ntfs_set_ea(inode, name, name_len, value, size, flags);
> >>>>  
> >>>>  out:
> >>>>  	return err;
> >>>> @@ -1006,26 +1004,26 @@ int ntfs_save_wsl_perm(struct inode *inode)
> >>>>  
> > 
> > do lock here and ...
> > 
> >>>>  	value = cpu_to_le32(i_uid_read(inode));
> >>>>  	err = ntfs_set_ea(inode, "$LXUID", sizeof("$LXUID") - 1, &value,
> >>>> -			  sizeof(value), 0, 0);
> >>>> +			  sizeof(value), 0);
> >>>>  	if (err)
> >>>>  		goto out;
> >>>>  
> >>>>  	value = cpu_to_le32(i_gid_read(inode));
> >>>>  	err = ntfs_set_ea(inode, "$LXGID", sizeof("$LXGID") - 1, &value,
> >>>> -			  sizeof(value), 0, 0);
> >>>> +			  sizeof(value), 0);
> >>>>  	if (err)
> >>>>  		goto out;
> >>>>  
> >>>>  	value = cpu_to_le32(inode->i_mode);
> >>>>  	err = ntfs_set_ea(inode, "$LXMOD", sizeof("$LXMOD") - 1, &value,
> >>>> -			  sizeof(value), 0, 0);
> >>>> +			  sizeof(value), 0);
> >>>>  	if (err)
> >>>>  		goto out;
> >>>>  
> >>>>  	if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) {
> >>>>  		value = cpu_to_le32(inode->i_rdev);
> >>>>  		err = ntfs_set_ea(inode, "$LXDEV", sizeof("$LXDEV") - 1, &value,
> >>>> -				  sizeof(value), 0, 0);
> >>>> +				  sizeof(value), 0);
> > 
> > unlock here. Of course unlock also in error path.
> > 
> >>>
> >>> Is this really that we can lock/unlock same lock 4 times in a row in a
> >>> ntfs_set_ea? This does not feel correct. 
> >>>
> >>>   Argillander
> >>>
> >>
> >> How it was working before d562e901f25d 
> >> "fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode":
> >>
> >> ntfs_create (lock mutex) =>
> >> ntfs_create_inode =>
> >> ntfs_save_wsl_perm (we are under lock here) =>
> >> return to ntfs_create and unlock
> >>
> >> How it works with d562e901f25d:
> >>
> >> ntfs_create => 
> >> ntfs_create_inode (lock in line 1201 file fs/ntfs3/inode.c 
> >> and unlock in line 1557) => 
> >> ntfs_save_wsl_perm (we aren't under lock here in line 1605)
> >>
> >> So we need to lock 4 times because there are 4 ntfs_set_ea calls.
> >> But now there can be done more work between those calls
> >> in other threads, locks became more granular.
> > 
> > Yeah but locking and locking 4 times when we can do it just ones is
> > quite waste. Please consider my suggestion above or tell what is wrong
> > with it. 
> > 
> >   Argillander
> > 
> 
> If I've understood correctly, you want to lock once in start of function 
> ntfs_save_wsl_perm and unlock at the end of it.
> It takes care of locks for those 4 calls to ntfs_set_ea.
> 
> But there are still other calls to ntfs_set_ea, that aren't protected
> in this case:
> function ntfs_set_acl_ex (line 603 file fs/ntfs3/xattr.c)
> err = ntfs_set_ea(inode, name, name_len, value, size, flags, locked);
> 
> This function called there:
> - function ntfs_set_acl  (line 621 file fs/ntfs3/xattr.c)
> return ntfs_set_acl_ex(mnt_userns, inode, acl, type);
> - function ntfs_init_acl (line 701 file fs/ntfs3/xattr.c)
> err = ntfs_set_acl_ex(mnt_userns, inode, default_acl,
> - function ntfs_init_acl (line 712 file fs/ntfs3/xattr.c)
> err = ntfs_set_acl_ex(mnt_userns, inode, acl,
> 
> So there are many entry points to ntfs_set_ea (and can be added more).
> Of course we can let int locked remain for these situations,
> but in my opinion it makes code a lot less readable
> (that was the reason to start looking into locking).

Yeah this what I was suggesting. And I totally agree that it is less
readable.

> I'm not sure, that winning some lock/unlocking
> in one specific scenario is worth it.

Still 4 locking / 4 unlocking in row is painfull. One option is to lock
outside of ntfs_set_acl if that is ok to you. If not maybe atleast add
todo comment to ntfs_set_wsl_perm that locking needs to be rethinked. I
know that there is lot of work to do with locks in here so maybe it is
not right time to nit pick about them.

> 
> >>
> >>>>  		if (err)
> >>>>  			goto out;
> >>>>  	}
> >>>> -- 
> >>>> 2.33.0
> >>>>
> >>>>