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

[PATCH v2 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.

V2:
  fixed typo.

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

Reviewed-by: Kari Argillander <kari.argillander@gmail.com>
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 v2 2/3] fs/ntfs3: Remove locked argument in ntfs_set_ea

[Konstantin Komarov]

We always need to lock now, because locks became smaller
(see d562e901f25d
"fs/ntfs3: 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 v2 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.
Thanks Joe Perches <joe@perches.com> for help.

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..5023d6f7e671 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 extended attributes must not exceed 64KiB");
 		err = -EFBIG; // -EINVAL?
 		goto out;
 	}
-	ea_info.size = cpu_to_le32(size);
 
 update_ea:
 
-- 
2.33.0

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

[Kari Argillander]

On Mon, Sep 27, 2021 at 06:26:26PM +0300, Konstantin Komarov wrote:
> Removed function, that already have been in kernel.
> Changed locking policy to fix some potential bugs.
> Changed code for readability.
> 
> V2:
>   fixed typo.

In the future please tell more closly. Now reviewr has to check
everything again. It is also good thing to write if someone suggest it.
Then that person can see right away that you change what he/she
suggested.

Also usually when someone comment something to previes series version
then you take all commenters to cc list. Usually reviewer will might
wanna give reviewed-by tag after you change what suggested.

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

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

[Kari Argillander]

On Mon, Sep 27, 2021 at 06:28:37PM +0300, Konstantin Komarov wrote:
> Make code more readable.
> Don't try to read zero bytes.
> Add warning when size of exteneded attribute exceeds limit.
> Thanks Joe Perches <joe@perches.com> for help.

Usually if someone review and suggest something small do not add this
kind of line to commit message. Also you need permission to add this. It
us same kind of situation when we add suggested-by tag. Linux
documentation stated that it cannot be there if we do not have
permission from other.

Also at least add that person email 'to line'. Sometimes if someone make
huge impact to patch you can ask and add this kind of line. But then
again it might make more sense to add it suggested or even signed off
tag depending in situation.

It can stay if Joe says it is ok.

> 
> 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..5023d6f7e671 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 extended attributes must not exceed 64KiB");
>  		err = -EFBIG; // -EINVAL?
>  		goto out;
>  	}
> -	ea_info.size = cpu_to_le32(size);
>  
>  update_ea:
>  
> -- 
> 2.33.0
> 
> 

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

[Kari Argillander]

Joe's address was wrong. Just resend.

On Mon, Sep 27, 2021 at 10:22:36PM +0300, Kari Argillander wrote:
> On Mon, Sep 27, 2021 at 06:28:37PM +0300, Konstantin Komarov wrote:
> > Make code more readable.
> > Don't try to read zero bytes.
> > Add warning when size of exteneded attribute exceeds limit.
> > Thanks Joe Perches <joe@perches.com> for help.
> 
> Usually if someone review and suggest something small do not add this
> kind of line to commit message. Also you need permission to add this. It
> us same kind of situation when we add suggested-by tag. Linux
> documentation stated that it cannot be there if we do not have
> permission from other.
> 
> Also at least add that person email 'to line'. Sometimes if someone make
> huge impact to patch you can ask and add this kind of line. But then
> again it might make more sense to add it suggested or even signed off
> tag depending in situation.
> 
> It can stay if Joe says it is ok.
> 
> > 
> > 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..5023d6f7e671 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 extended attributes must not exceed 64KiB");
> >  		err = -EFBIG; // -EINVAL?
> >  		goto out;
> >  	}
> > -	ea_info.size = cpu_to_le32(size);
> >  
> >  update_ea:
> >  
> > -- 
> > 2.33.0
> > 
> >