[PATCH 0/3] fs/ntfs3: Refactoring and bugfix

[PATCH 0/3] fs/ntfs3: Refactoring and bugfix

[Almaz Alexandrovich]

There are 3 commits.
First - to improve code readability.
Second - bugfix for xfstest (fixing wrong logic).
Third - restructuring function logic so
we can restore inode in some error cases.

Konstantin Komarov (3):
   fs/ntfs3: Refactoring of indx_find function
   fs/ntfs3: Fix double free on remount
   fs/ntfs3: Refactor ni_try_remove_attr_list function

  fs/ntfs3/frecord.c | 49 ++++++++++++++++++++++++++++++++++------------
  fs/ntfs3/index.c   | 20 ++++++-------------
  fs/ntfs3/record.c  |  5 ++---
  fs/ntfs3/super.c   |  8 ++++----
  4 files changed, 49 insertions(+), 33 deletions(-)

-- 
2.36.1

[PATCH 1/3] fs/ntfs3: Refactoring of indx_find function

[Almaz Alexandrovich]

This commit makes function a bit more readable

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

diff --git a/fs/ntfs3/index.c b/fs/ntfs3/index.c
index 6f81e3a49abf..511f872b6650 100644
--- a/fs/ntfs3/index.c
+++ b/fs/ntfs3/index.c
@@ -1042,19 +1042,16 @@ int indx_find(struct ntfs_index *indx, struct ntfs_inode *ni,
  {
  	int err;
  	struct NTFS_DE *e;
-	const struct INDEX_HDR *hdr;
  	struct indx_node *node;
  
  	if (!root)
  		root = indx_get_root(&ni->dir, ni, NULL, NULL);
  
  	if (!root) {
-		err = -EINVAL;
-		goto out;
+		/* Should not happed. */
+		return -EINVAL;
  	}
  
-	hdr = &root->ihdr;
-
  	/* Check cache. */
  	e = fnd->level ? fnd->de[fnd->level - 1] : fnd->root_de;
  	if (e && !de_is_last(e) &&
@@ -1068,39 +1065,34 @@ int indx_find(struct ntfs_index *indx, struct ntfs_inode *ni,
  	fnd_clear(fnd);
  
  	/* Lookup entry that is <= to the search value. */
-	e = hdr_find_e(indx, hdr, key, key_len, ctx, diff);
+	e = hdr_find_e(indx, &root->ihdr, key, key_len, ctx, diff);
  	if (!e)
  		return -EINVAL;
  
  	fnd->root_de = e;
-	err = 0;
  
  	for (;;) {
  		node = NULL;
  		if (*diff >= 0 || !de_has_vcn_ex(e)) {
  			*entry = e;
-			goto out;
+			return 0;
  		}
  
  		/* Read next level. */
  		err = indx_read(indx, ni, de_get_vbn(e), &node);
  		if (err)
-			goto out;
+			return err;
  
  		/* Lookup entry that is <= to the search value. */
  		e = hdr_find_e(indx, &node->index->ihdr, key, key_len, ctx,
  			       diff);
  		if (!e) {
-			err = -EINVAL;
  			put_indx_node(node);
-			goto out;
+			return -EINVAL;
  		}
  
  		fnd_push(fnd, node, e);
  	}
-
-out:
-	return err;
  }
  
  int indx_find_sort(struct ntfs_index *indx, struct ntfs_inode *ni,
-- 
2.36.1

Re: [PATCH 1/3] fs/ntfs3: Refactoring of indx_find function

[Joe Perches]

On Fri, 2022-05-27 at 17:21 +0300, Almaz Alexandrovich wrote:
> This commit makes function a bit more readable

trivia:

> diff --git a/fs/ntfs3/index.c b/fs/ntfs3/index.c
[]
> @@ -1042,19 +1042,16 @@ int indx_find(struct ntfs_index *indx, struct ntfs_inode *ni,
>   {
>   	int err;
>   	struct NTFS_DE *e;
> -	const struct INDEX_HDR *hdr;
>   	struct indx_node *node;
>   
>   	if (!root)
>   		root = indx_get_root(&ni->dir, ni, NULL, NULL);
>   
>   	if (!root) {
> -		err = -EINVAL;
> -		goto out;
> +		/* Should not happed. */
> +		return -EINVAL;

s/happed/happen/

>   	for (;;) {
>   		node = NULL;
>   		if (*diff >= 0 || !de_has_vcn_ex(e)) {
>   			*entry = e;
> -			goto out;
> +			return 0;
>   		}

might be nicer with a break; or a while like

	while (*diff < 0 && de_has_vcn_ex(e)) {
		node = NULL;


>   		/* Read next level. */
>   		err = indx_read(indx, ni, de_get_vbn(e), &node);
>   		if (err)
> -			goto out;
> +			return err;
>   
>   		/* Lookup entry that is <= to the search value. */
>   		e = hdr_find_e(indx, &node->index->ihdr, key, key_len, ctx,
>   			       diff);
>   		if (!e) {
> -			err = -EINVAL;
>   			put_indx_node(node);
> -			goto out;
> +			return -EINVAL;
>   		}
>   
>   		fnd_push(fnd, node, e);
>   	}
> -
> -out:
> -	return err;

and a return 0;

or
	*entry = e;
	return 0;

so it appears that the function has a typical return value.

Re: [PATCH 1/3] fs/ntfs3: Refactoring of indx_find function

[Konstantin Komarov]

Hello.

Thanks for your input.
It's nice to have a clear typical return value, so I've updated patch.


On 5/27/22 19:07, Joe Perches wrote:
> On Fri, 2022-05-27 at 17:21 +0300, Almaz Alexandrovich wrote:
>> This commit makes function a bit more readable
> 
> trivia:
> 
>> diff --git a/fs/ntfs3/index.c b/fs/ntfs3/index.c
> []
>> @@ -1042,19 +1042,16 @@ int indx_find(struct ntfs_index *indx, struct ntfs_inode *ni,
>>    {
>>    	int err;
>>    	struct NTFS_DE *e;
>> -	const struct INDEX_HDR *hdr;
>>    	struct indx_node *node;
>>    
>>    	if (!root)
>>    		root = indx_get_root(&ni->dir, ni, NULL, NULL);
>>    
>>    	if (!root) {
>> -		err = -EINVAL;
>> -		goto out;
>> +		/* Should not happed. */
>> +		return -EINVAL;
> 
> s/happed/happen/
> 
>>    	for (;;) {
>>    		node = NULL;
>>    		if (*diff >= 0 || !de_has_vcn_ex(e)) {
>>    			*entry = e;
>> -			goto out;
>> +			return 0;
>>    		}
> 
> might be nicer with a break; or a while like
> 
> 	while (*diff < 0 && de_has_vcn_ex(e)) {
> 		node = NULL;
> 
> 
>>    		/* Read next level. */
>>    		err = indx_read(indx, ni, de_get_vbn(e), &node);
>>    		if (err)
>> -			goto out;
>> +			return err;
>>    
>>    		/* Lookup entry that is <= to the search value. */
>>    		e = hdr_find_e(indx, &node->index->ihdr, key, key_len, ctx,
>>    			       diff);
>>    		if (!e) {
>> -			err = -EINVAL;
>>    			put_indx_node(node);
>> -			goto out;
>> +			return -EINVAL;
>>    		}
>>    
>>    		fnd_push(fnd, node, e);
>>    	}
>> -
>> -out:
>> -	return err;
> 
> and a return 0;
> 
> or
> 	*entry = e;
> 	return 0;
> 
> so it appears that the function has a typical return value.
> 

Re: [PATCH 1/3] fs/ntfs3: Refactoring of indx_find function

[Dave Chinner]

On Fri, May 27, 2022 at 05:21:03PM +0300, Almaz Alexandrovich wrote:
> This commit makes function a bit more readable
> 
> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>

This looks wrong. The email is from

 "From: Almaz Alexandrovich <almaz.alexandrovich@paragon-software.com>"

So it looks like the S-o-B has the wrong email address in it. All
the patches have this same problem.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/3] fs/ntfs3: Refactoring of indx_find function

[Konstantin Komarov]

This was caused by an incorrect account setting in Thunderbird.
Thanks for catching this.

On 5/28/22 03:06, Dave Chinner wrote:
> On Fri, May 27, 2022 at 05:21:03PM +0300, Almaz Alexandrovich wrote:
>> This commit makes function a bit more readable
>>
>> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
> 
> This looks wrong. The email is from
> 
>   "From: Almaz Alexandrovich <almaz.alexandrovich@paragon-software.com>"
> 
> So it looks like the S-o-B has the wrong email address in it. All
> the patches have this same problem.
> 
> Cheers,
> 
> Dave.

[PATCH 2/3] fs/ntfs3: Fix double free on remount

[Almaz Alexandrovich]

Pointer to options was freed twice on remount
Fixes xfstest generic/361
Fixes: 82cae269cfa9 ("fs/ntfs3: Add initialization of super block")

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

diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
index d41d76979e12..697a84ed395e 100644
--- a/fs/ntfs3/super.c
+++ b/fs/ntfs3/super.c
@@ -30,6 +30,7 @@
  #include <linux/fs_context.h>
  #include <linux/fs_parser.h>
  #include <linux/log2.h>
+#include <linux/minmax.h>
  #include <linux/module.h>
  #include <linux/nls.h>
  #include <linux/seq_file.h>
@@ -390,7 +391,7 @@ static int ntfs_fs_reconfigure(struct fs_context *fc)
  		return -EINVAL;
  	}
  
-	memcpy(sbi->options, new_opts, sizeof(*new_opts));
+	swap(sbi->options, fc->fs_private);
  
  	return 0;
  }
@@ -897,6 +898,8 @@ static int ntfs_fill_super(struct super_block *sb, struct fs_context *fc)
  	ref.high = 0;
  
  	sbi->sb = sb;
+	sbi->options = fc->fs_private;
+	fc->fs_private = NULL;
  	sb->s_flags |= SB_NODIRATIME;
  	sb->s_magic = 0x7366746e; // "ntfs"
  	sb->s_op = &ntfs_sops;
@@ -1260,8 +1263,6 @@ static int ntfs_fill_super(struct super_block *sb, struct fs_context *fc)
  		goto put_inode_out;
  	}
  
-	fc->fs_private = NULL;
-
  	return 0;
  
  put_inode_out:
@@ -1414,7 +1415,6 @@ static int ntfs_init_fs_context(struct fs_context *fc)
  	mutex_init(&sbi->compress.mtx_lzx);
  #endif
  
-	sbi->options = opts;
  	fc->s_fs_info = sbi;
  ok:
  	fc->fs_private = opts;
-- 
2.36.1

[PATCH 3/3] fs/ntfs3: Refactor ni_try_remove_attr_list function

[Almaz Alexandrovich]

Now we save a copy of primary record for restoration.
Also now we remove all attributes from subrecords.

Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
---
  fs/ntfs3/frecord.c | 49 ++++++++++++++++++++++++++++++++++------------
  fs/ntfs3/record.c  |  5 ++---
  2 files changed, 39 insertions(+), 15 deletions(-)

diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c
index 18842998c8fa..3576268ee0a1 100644
--- a/fs/ntfs3/frecord.c
+++ b/fs/ntfs3/frecord.c
@@ -7,6 +7,7 @@
  
  #include <linux/fiemap.h>
  #include <linux/fs.h>
+#include <linux/minmax.h>
  #include <linux/vmalloc.h>
  
  #include "debug.h"
@@ -649,6 +650,7 @@ static int ni_try_remove_attr_list(struct ntfs_inode *ni)
  	struct mft_inode *mi;
  	u32 asize, free;
  	struct MFT_REF ref;
+	struct MFT_REC *mrec;
  	__le16 id;
  
  	if (!ni->attr_list.dirty)
@@ -692,11 +694,17 @@ static int ni_try_remove_attr_list(struct ntfs_inode *ni)
  		free -= asize;
  	}
  
+	/* Make a copy of primary record to restore if error. */
+	mrec = kmemdup(ni->mi.mrec, sbi->record_size, GFP_NOFS);
+	if (!mrec)
+		return 0; /* Not critical. */
+
  	/* It seems that attribute list can be removed from primary record. */
  	mi_remove_attr(NULL, &ni->mi, attr_list);
  
  	/*
-	 * Repeat the cycle above and move all attributes to primary record.
+	 * Repeat the cycle above and copy all attributes to primary record.
+	 * Do not remove original attributes from subrecords!
  	 * It should be success!
  	 */
  	le = NULL;
@@ -707,14 +715,14 @@ static int ni_try_remove_attr_list(struct ntfs_inode *ni)
  		mi = ni_find_mi(ni, ino_get(&le->ref));
  		if (!mi) {
  			/* Should never happened, 'cause already checked. */
-			goto bad;
+			goto out;
  		}
  
  		attr = mi_find_attr(mi, NULL, le->type, le_name(le),
  				    le->name_len, &le->id);
  		if (!attr) {
  			/* Should never happened, 'cause already checked. */
-			goto bad;
+			goto out;
  		}
  		asize = le32_to_cpu(attr->size);
  
@@ -724,18 +732,33 @@ static int ni_try_remove_attr_list(struct ntfs_inode *ni)
  					  le16_to_cpu(attr->name_off));
  		if (!attr_ins) {
  			/*
-			 * Internal error.
-			 * Either no space in primary record (already checked).
-			 * Either tried to insert another
-			 * non indexed attribute (logic error).
+			 * No space in primary record (already checked).
  			 */
-			goto bad;
+			goto out;
  		}
  
  		/* Copy all except id. */
  		id = attr_ins->id;
  		memcpy(attr_ins, attr, asize);
  		attr_ins->id = id;
+	}
+
+	/*
+	 * Repeat the cycle above and remove all attributes from subrecords.
+	 */
+	le = NULL;
+	while ((le = al_enumerate(ni, le))) {
+		if (!memcmp(&le->ref, &ref, sizeof(ref)))
+			continue;
+
+		mi = ni_find_mi(ni, ino_get(&le->ref));
+		if (!mi)
+			continue;
+
+		attr = mi_find_attr(mi, NULL, le->type, le_name(le),
+				    le->name_len, &le->id);
+		if (!attr)
+			continue;
  
  		/* Remove from original record. */
  		mi_remove_attr(NULL, mi, attr);
@@ -748,11 +771,13 @@ static int ni_try_remove_attr_list(struct ntfs_inode *ni)
  	ni->attr_list.le = NULL;
  	ni->attr_list.dirty = false;
  
+	kfree(mrec);
+	return 0;
+out:
+	/* Restore primary record. */
+	swap(mrec, ni->mi.mrec);
+	kfree(mrec);
  	return 0;
-bad:
-	ntfs_inode_err(&ni->vfs_inode, "Internal error");
-	make_bad_inode(&ni->vfs_inode);
-	return -EINVAL;
  }
  
  /*
diff --git a/fs/ntfs3/record.c b/fs/ntfs3/record.c
index 861e35791506..8fe0a876400a 100644
--- a/fs/ntfs3/record.c
+++ b/fs/ntfs3/record.c
@@ -445,12 +445,11 @@ struct ATTRIB *mi_insert_attr(struct mft_inode *mi, enum ATTR_TYPE type,
  	attr = NULL;
  	while ((attr = mi_enum_attr(mi, attr))) {
  		diff = compare_attr(attr, type, name, name_len, upcase);
-		if (diff > 0)
-			break;
+
  		if (diff < 0)
  			continue;
  
-		if (!is_attr_indexed(attr))
+		if (!diff && !is_attr_indexed(attr))
  			return NULL;
  		break;
  	}
-- 
2.36.1