[PATCH 1/2] exfat: add NameLength check when extracting name

[PATCH 1/2] exfat: add NameLength check when extracting name

[Tetsuhiro Kohada]

The current implementation doesn't care NameLength when extracting
the name from Name dir-entries, so the name may be incorrect.
(Without null-termination, Insufficient Name dir-entry, etc)
Add a NameLength check when extracting the name from Name dir-entries
to extract correct name.
And, change to get the information of file/stream-ext dir-entries
via the member variable of exfat_entry_set_cache.

** This patch depends on:
  '[PATCH v3] exfat: integrates dir-entry getting and validation'.

Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com>
---
 fs/exfat/dir.c | 81 ++++++++++++++++++++++++--------------------------
 1 file changed, 39 insertions(+), 42 deletions(-)

diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c
index 91cdbede0fd1..545bb73b95e9 100644
--- a/fs/exfat/dir.c
+++ b/fs/exfat/dir.c
@@ -28,16 +28,15 @@ static int exfat_extract_uni_name(struct exfat_dentry *ep,
 
 }
 
-static void exfat_get_uniname_from_ext_entry(struct super_block *sb,
-		struct exfat_chain *p_dir, int entry, unsigned short *uniname)
+static int exfat_get_uniname_from_name_entries(struct exfat_entry_set_cache *es,
+		struct exfat_uni_name *uniname)
 {
-	int i;
-	struct exfat_entry_set_cache *es;
+	int n, l, i;
 	struct exfat_dentry *ep;
 
-	es = exfat_get_dentry_set(sb, p_dir, entry, ES_ALL_ENTRIES);
-	if (!es)
-		return;
+	uniname->name_len = es->de_stream->name_len;
+	if (uniname->name_len == 0)
+		return -EIO;
 
 	/*
 	 * First entry  : file entry
@@ -45,14 +44,15 @@ static void exfat_get_uniname_from_ext_entry(struct super_block *sb,
 	 * Third entry  : first file-name entry
 	 * So, the index of first file-name dentry should start from 2.
 	 */
-
-	i = 2;
-	while ((ep = exfat_get_validated_dentry(es, i++, TYPE_NAME))) {
-		exfat_extract_uni_name(ep, uniname);
-		uniname += EXFAT_FILE_NAME_LEN;
+	for (l = 0, n = 2; l < uniname->name_len; n++) {
+		ep = exfat_get_validated_dentry(es, n, TYPE_NAME);
+		if (!ep)
+			return -EIO;
+		for (i = 0; l < uniname->name_len && i < EXFAT_FILE_NAME_LEN; i++, l++)
+			uniname->name[l] = le16_to_cpu(ep->dentry.name.unicode_0_14[i]);
 	}
-
-	exfat_free_dentry_set(es, false);
+	uniname->name[l] = 0;
+	return 0;
 }
 
 /* read a directory entry from the opened directory */
@@ -63,6 +63,7 @@ static int exfat_readdir(struct inode *inode, struct exfat_dir_entry *dir_entry)
 	sector_t sector;
 	struct exfat_chain dir, clu;
 	struct exfat_uni_name uni_name;
+	struct exfat_entry_set_cache *es;
 	struct exfat_dentry *ep;
 	struct super_block *sb = inode->i_sb;
 	struct exfat_sb_info *sbi = EXFAT_SB(sb);
@@ -114,47 +115,43 @@ static int exfat_readdir(struct inode *inode, struct exfat_dir_entry *dir_entry)
 				return -EIO;
 
 			type = exfat_get_entry_type(ep);
-			if (type == TYPE_UNUSED) {
-				brelse(bh);
+			brelse(bh);
+
+			if (type == TYPE_UNUSED)
 				break;
-			}
 
-			if (type != TYPE_FILE && type != TYPE_DIR) {
-				brelse(bh);
+			if (type != TYPE_FILE && type != TYPE_DIR)
 				continue;
-			}
 
-			dir_entry->attr = le16_to_cpu(ep->dentry.file.attr);
+			es = exfat_get_dentry_set(sb, &dir, dentry, ES_ALL_ENTRIES);
+			if (!es)
+				return -EIO;
+
+			dir_entry->attr = le16_to_cpu(es->de_file->attr);
 			exfat_get_entry_time(sbi, &dir_entry->crtime,
-					ep->dentry.file.create_tz,
-					ep->dentry.file.create_time,
-					ep->dentry.file.create_date,
-					ep->dentry.file.create_time_cs);
+					es->de_file->create_tz,
+					es->de_file->create_time,
+					es->de_file->create_date,
+					es->de_file->create_time_cs);
 			exfat_get_entry_time(sbi, &dir_entry->mtime,
-					ep->dentry.file.modify_tz,
-					ep->dentry.file.modify_time,
-					ep->dentry.file.modify_date,
-					ep->dentry.file.modify_time_cs);
+					es->de_file->modify_tz,
+					es->de_file->modify_time,
+					es->de_file->modify_date,
+					es->de_file->modify_time_cs);
 			exfat_get_entry_time(sbi, &dir_entry->atime,
-					ep->dentry.file.access_tz,
-					ep->dentry.file.access_time,
-					ep->dentry.file.access_date,
+					es->de_file->access_tz,
+					es->de_file->access_time,
+					es->de_file->access_date,
 					0);
 
-			*uni_name.name = 0x0;
-			exfat_get_uniname_from_ext_entry(sb, &dir, dentry,
-				uni_name.name);
+			dir_entry->size = le64_to_cpu(es->de_stream->valid_size);
+
+			exfat_get_uniname_from_name_entries(es, &uni_name);
 			exfat_utf16_to_nls(sb, &uni_name,
 				dir_entry->namebuf.lfn,
 				dir_entry->namebuf.lfnbuf_len);
-			brelse(bh);
 
-			ep = exfat_get_dentry(sb, &clu, i + 1, &bh, NULL);
-			if (!ep)
-				return -EIO;
-			dir_entry->size =
-				le64_to_cpu(ep->dentry.stream.valid_size);
-			brelse(bh);
+			exfat_free_dentry_set(es, false);
 
 			ei->hint_bmap.off = dentry >> dentries_per_clu_bits;
 			ei->hint_bmap.clu = clu.dir;
-- 
2.25.1

[PATCH 2/2] exfat: unify name extraction

[Tetsuhiro Kohada]

Name extraction in exfat_find_dir_entry() also doesn't care NameLength,
so the name may be incorrect.
Replace the name extraction in exfat_find_dir_entry() with using
exfat_entry_set_cache and exfat_get_uniname_from_name_entries(),
like exfat_readdir().
Replace the name extraction with using exfat_entry_set_cache and
exfat_get_uniname_from_name_entries(), like exfat_readdir().
And, remove unused functions/parameters.

** This patch depends on:
  '[PATCH v3] exfat: integrates dir-entry getting and validation'.

Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com>
---
 fs/exfat/dir.c      | 161 ++++++++++----------------------------------
 fs/exfat/exfat_fs.h |   2 +-
 fs/exfat/namei.c    |   4 +-
 3 files changed, 38 insertions(+), 129 deletions(-)

diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c
index 545bb73b95e9..c9715c7a55a1 100644
--- a/fs/exfat/dir.c
+++ b/fs/exfat/dir.c
@@ -10,24 +10,6 @@
 #include "exfat_raw.h"
 #include "exfat_fs.h"
 
-static int exfat_extract_uni_name(struct exfat_dentry *ep,
-		unsigned short *uniname)
-{
-	int i, len = 0;
-
-	for (i = 0; i < EXFAT_FILE_NAME_LEN; i++) {
-		*uniname = le16_to_cpu(ep->dentry.name.unicode_0_14[i]);
-		if (*uniname == 0x0)
-			return len;
-		uniname++;
-		len++;
-	}
-
-	*uniname = 0x0;
-	return len;
-
-}
-
 static int exfat_get_uniname_from_name_entries(struct exfat_entry_set_cache *es,
 		struct exfat_uni_name *uniname)
 {
@@ -869,13 +851,6 @@ struct exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb,
 	return NULL;
 }
 
-enum {
-	DIRENT_STEP_FILE,
-	DIRENT_STEP_STRM,
-	DIRENT_STEP_NAME,
-	DIRENT_STEP_SECD,
-};
-
 /*
  * return values:
  *   >= 0	: return dir entiry position with the name in dir
@@ -885,13 +860,12 @@ enum {
  */
 int exfat_find_dir_entry(struct super_block *sb, struct exfat_inode_info *ei,
 		struct exfat_chain *p_dir, struct exfat_uni_name *p_uniname,
-		int num_entries, unsigned int type)
+		int num_entries)
 {
-	int i, rewind = 0, dentry = 0, end_eidx = 0, num_ext = 0, len;
-	int order, step, name_len = 0;
+	int i, rewind = 0, dentry = 0, end_eidx = 0, num_ext = 0;
+	int name_len = 0;
 	int dentries_per_clu, num_empty = 0;
 	unsigned int entry_type;
-	unsigned short *uniname = NULL;
 	struct exfat_chain clu;
 	struct exfat_hint *hint_stat = &ei->hint_stat;
 	struct exfat_hint_femp candi_empty;
@@ -909,27 +883,33 @@ int exfat_find_dir_entry(struct super_block *sb, struct exfat_inode_info *ei,
 
 	candi_empty.eidx = EXFAT_HINT_NONE;
 rewind:
-	order = 0;
-	step = DIRENT_STEP_FILE;
 	while (clu.dir != EXFAT_EOF_CLUSTER) {
 		i = dentry & (dentries_per_clu - 1);
 		for (; i < dentries_per_clu; i++, dentry++) {
 			struct exfat_dentry *ep;
 			struct buffer_head *bh;
+			struct exfat_entry_set_cache *es;
+			struct exfat_uni_name uni_name;
+			u16 name_hash;
 
 			if (rewind && dentry == end_eidx)
 				goto not_found;
 
+			/* skip secondary dir-entries in previous dir-entry set */
+			if (num_ext) {
+				num_ext--;
+				continue;
+			}
+
 			ep = exfat_get_dentry(sb, &clu, i, &bh, NULL);
 			if (!ep)
 				return -EIO;
 
 			entry_type = exfat_get_entry_type(ep);
+			brelse(bh);
 
 			if (entry_type == TYPE_UNUSED ||
 			    entry_type == TYPE_DELETED) {
-				step = DIRENT_STEP_FILE;
-
 				num_empty++;
 				if (candi_empty.eidx == EXFAT_HINT_NONE &&
 						num_empty == 1) {
@@ -954,7 +934,6 @@ int exfat_find_dir_entry(struct super_block *sb, struct exfat_inode_info *ei,
 					}
 				}
 
-				brelse(bh);
 				if (entry_type == TYPE_UNUSED)
 					goto not_found;
 				continue;
@@ -963,80 +942,38 @@ int exfat_find_dir_entry(struct super_block *sb, struct exfat_inode_info *ei,
 			num_empty = 0;
 			candi_empty.eidx = EXFAT_HINT_NONE;
 
-			if (entry_type == TYPE_FILE || entry_type == TYPE_DIR) {
-				step = DIRENT_STEP_FILE;
-				if (type == TYPE_ALL || type == entry_type) {
-					num_ext = ep->dentry.file.num_ext;
-					step = DIRENT_STEP_STRM;
-				}
-				brelse(bh);
+			if (entry_type != TYPE_FILE && entry_type != TYPE_DIR)
 				continue;
-			}
-
-			if (entry_type == TYPE_STREAM) {
-				u16 name_hash;
 
-				if (step != DIRENT_STEP_STRM) {
-					step = DIRENT_STEP_FILE;
-					brelse(bh);
-					continue;
-				}
-				step = DIRENT_STEP_FILE;
-				name_hash = le16_to_cpu(
-						ep->dentry.stream.name_hash);
-				if (p_uniname->name_hash == name_hash &&
-				    p_uniname->name_len ==
-						ep->dentry.stream.name_len) {
-					step = DIRENT_STEP_NAME;
-					order = 1;
-					name_len = 0;
-				}
-				brelse(bh);
+			es = exfat_get_dentry_set(sb, &ei->dir, dentry, ES_2_ENTRIES);
+			if (!es)
 				continue;
-			}
 
-			brelse(bh);
-			if (entry_type == TYPE_NAME) {
-				unsigned short entry_uniname[16], unichar;
-
-				if (step != DIRENT_STEP_NAME) {
-					step = DIRENT_STEP_FILE;
-					continue;
-				}
-
-				if (++order == 2)
-					uniname = p_uniname->name;
-				else
-					uniname += EXFAT_FILE_NAME_LEN;
-
-				len = exfat_extract_uni_name(ep, entry_uniname);
-				name_len += len;
-
-				unichar = *(uniname+len);
-				*(uniname+len) = 0x0;
+			num_ext = es->de_file->num_ext;
+			name_hash = le16_to_cpu(es->de_stream->name_hash);
+			name_len = es->de_stream->name_len;
+			exfat_free_dentry_set(es, false);
 
-				if (exfat_uniname_ncmp(sb, uniname,
-					entry_uniname, len)) {
-					step = DIRENT_STEP_FILE;
-				} else if (p_uniname->name_len == name_len) {
-					if (order == num_ext)
-						goto found;
-					step = DIRENT_STEP_SECD;
-				}
+			if (p_uniname->name_hash != name_hash ||
+			    p_uniname->name_len != name_len)
+				continue;
 
-				*(uniname+len) = unichar;
+			es = exfat_get_dentry_set(sb, &ei->dir, dentry, ES_ALL_ENTRIES);
+			if (!es)
 				continue;
-			}
 
-			if (entry_type &
-					(TYPE_CRITICAL_SEC | TYPE_BENIGN_SEC)) {
-				if (step == DIRENT_STEP_SECD) {
-					if (++order == num_ext)
-						goto found;
-					continue;
-				}
+			exfat_get_uniname_from_name_entries(es, &uni_name);
+			exfat_free_dentry_set(es, false);
+
+			if (!exfat_uniname_ncmp(sb,
+						p_uniname->name,
+						uni_name.name,
+						name_len)) {
+				/* set the last used position as hint */
+				hint_stat->clu = clu.dir;
+				hint_stat->eidx = dentry;
+				return dentry;
 			}
-			step = DIRENT_STEP_FILE;
 		}
 
 		if (clu.flags == ALLOC_NO_FAT_CHAIN) {
@@ -1069,32 +1006,6 @@ int exfat_find_dir_entry(struct super_block *sb, struct exfat_inode_info *ei,
 	hint_stat->clu = p_dir->dir;
 	hint_stat->eidx = 0;
 	return -ENOENT;
-
-found:
-	/* next dentry we'll find is out of this cluster */
-	if (!((dentry + 1) & (dentries_per_clu - 1))) {
-		int ret = 0;
-
-		if (clu.flags == ALLOC_NO_FAT_CHAIN) {
-			if (--clu.size > 0)
-				clu.dir++;
-			else
-				clu.dir = EXFAT_EOF_CLUSTER;
-		} else {
-			ret = exfat_get_next_cluster(sb, &clu.dir);
-		}
-
-		if (ret || clu.dir == EXFAT_EOF_CLUSTER) {
-			/* just initialized hint_stat */
-			hint_stat->clu = p_dir->dir;
-			hint_stat->eidx = 0;
-			return (dentry - num_ext);
-		}
-	}
-
-	hint_stat->clu = clu.dir;
-	hint_stat->eidx = dentry + 1;
-	return dentry - num_ext;
 }
 
 int exfat_count_ext_entries(struct super_block *sb, struct exfat_chain *p_dir,
diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
index b88b7abc25bd..62a4768a4f6e 100644
--- a/fs/exfat/exfat_fs.h
+++ b/fs/exfat/exfat_fs.h
@@ -456,7 +456,7 @@ void exfat_update_dir_chksum_with_entry_set(struct exfat_entry_set_cache *es);
 int exfat_calc_num_entries(struct exfat_uni_name *p_uniname);
 int exfat_find_dir_entry(struct super_block *sb, struct exfat_inode_info *ei,
 		struct exfat_chain *p_dir, struct exfat_uni_name *p_uniname,
-		int num_entries, unsigned int type);
+		int num_entries);
 int exfat_alloc_new_dir(struct inode *inode, struct exfat_chain *clu);
 int exfat_find_location(struct super_block *sb, struct exfat_chain *p_dir,
 		int entry, sector_t *sector, int *offset);
diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c
index a65d60ef93f4..c59d523547ca 100644
--- a/fs/exfat/namei.c
+++ b/fs/exfat/namei.c
@@ -625,9 +625,7 @@ static int exfat_find(struct inode *dir, struct qstr *qname,
 	}
 
 	/* search the file name for directories */
-	dentry = exfat_find_dir_entry(sb, ei, &cdir, &uni_name,
-			num_entries, TYPE_ALL);
-
+	dentry = exfat_find_dir_entry(sb, ei, &cdir, &uni_name, num_entries);
 	if ((dentry < 0) && (dentry != -EEXIST))
 		return dentry; /* -error value */
 
-- 
2.25.1

RE: [PATCH 2/2] exfat: unify name extraction

[Sungjong Seo]

> Name extraction in exfat_find_dir_entry() also doesn't care NameLength, so
> the name may be incorrect.
> Replace the name extraction in exfat_find_dir_entry() with using
> exfat_entry_set_cache and exfat_get_uniname_from_name_entries(),
> like exfat_readdir().
> Replace the name extraction with using exfat_entry_set_cache and
> exfat_get_uniname_from_name_entries(), like exfat_readdir().
> And, remove unused functions/parameters.
> 
> ** This patch depends on:
>   '[PATCH v3] exfat: integrates dir-entry getting and validation'.
> 
> Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com>
> ---
>  fs/exfat/dir.c      | 161 ++++++++++----------------------------------
>  fs/exfat/exfat_fs.h |   2 +-
>  fs/exfat/namei.c    |   4 +-
>  3 files changed, 38 insertions(+), 129 deletions(-)
> 
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
> 545bb73b95e9..c9715c7a55a1 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -10,24 +10,6 @@
>  #include "exfat_raw.h"
>  #include "exfat_fs.h"
[snip]
> @@ -963,80 +942,38 @@ int exfat_find_dir_entry(struct super_block *sb,
> struct exfat_inode_info *ei,
>  			num_empty = 0;
>  			candi_empty.eidx = EXFAT_HINT_NONE;
> 
[snip]
> 
> -			if (entry_type &
> -					(TYPE_CRITICAL_SEC |
TYPE_BENIGN_SEC)) {
> -				if (step == DIRENT_STEP_SECD) {
> -					if (++order == num_ext)
> -						goto found;
> -					continue;
> -				}
> +			exfat_get_uniname_from_name_entries(es, &uni_name);

It is needed to check a return value.

> +			exfat_free_dentry_set(es, false);
> +
> +			if (!exfat_uniname_ncmp(sb,
> +						p_uniname->name,
> +						uni_name.name,
> +						name_len)) {
> +				/* set the last used position as hint */
> +				hint_stat->clu = clu.dir;
> +				hint_stat->eidx = dentry;

eidx and clu of hint_stat should have one for the next entry we'll start
looking for.
Did you intentionally change the concept?

> +				return dentry;
>  			}
> -			step = DIRENT_STEP_FILE;
>  		}
> 
>  		if (clu.flags == ALLOC_NO_FAT_CHAIN) { @@ -1069,32 +1006,6
> @@ int exfat_find_dir_entry(struct super_block *sb, struct
> exfat_inode_info *ei,
>  	hint_stat->clu = p_dir->dir;
>  	hint_stat->eidx = 0;
>  	return -ENOENT;
> -
> -found:
> -	/* next dentry we'll find is out of this cluster */
> -	if (!((dentry + 1) & (dentries_per_clu - 1))) {
> -		int ret = 0;
> -
> -		if (clu.flags == ALLOC_NO_FAT_CHAIN) {
> -			if (--clu.size > 0)
> -				clu.dir++;
> -			else
> -				clu.dir = EXFAT_EOF_CLUSTER;
> -		} else {
> -			ret = exfat_get_next_cluster(sb, &clu.dir);
> -		}
> -
> -		if (ret || clu.dir == EXFAT_EOF_CLUSTER) {
> -			/* just initialized hint_stat */
> -			hint_stat->clu = p_dir->dir;
> -			hint_stat->eidx = 0;
> -			return (dentry - num_ext);
> -		}
> -	}
> -
> -	hint_stat->clu = clu.dir;
> -	hint_stat->eidx = dentry + 1;
> -	return dentry - num_ext;
>  }
> 
>  int exfat_count_ext_entries(struct super_block *sb, struct exfat_chain
> *p_dir, diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h index
> b88b7abc25bd..62a4768a4f6e 100644
> --- a/fs/exfat/exfat_fs.h
> +++ b/fs/exfat/exfat_fs.h
> @@ -456,7 +456,7 @@ void exfat_update_dir_chksum_with_entry_set(struct
> exfat_entry_set_cache *es);  int exfat_calc_num_entries(struct
> exfat_uni_name *p_uniname);  int exfat_find_dir_entry(struct super_block
> *sb, struct exfat_inode_info *ei,
>  		struct exfat_chain *p_dir, struct exfat_uni_name *p_uniname,
> -		int num_entries, unsigned int type);
> +		int num_entries);
>  int exfat_alloc_new_dir(struct inode *inode, struct exfat_chain *clu);
> int exfat_find_location(struct super_block *sb, struct exfat_chain *p_dir,
>  		int entry, sector_t *sector, int *offset); diff --git
> a/fs/exfat/namei.c b/fs/exfat/namei.c index a65d60ef93f4..c59d523547ca
> 100644
> --- a/fs/exfat/namei.c
> +++ b/fs/exfat/namei.c
> @@ -625,9 +625,7 @@ static int exfat_find(struct inode *dir, struct qstr
> *qname,
>  	}
> 
>  	/* search the file name for directories */
> -	dentry = exfat_find_dir_entry(sb, ei, &cdir, &uni_name,
> -			num_entries, TYPE_ALL);
> -
> +	dentry = exfat_find_dir_entry(sb, ei, &cdir, &uni_name,
> num_entries);
>  	if ((dentry < 0) && (dentry != -EEXIST))
>  		return dentry; /* -error value */
> 
> --
> 2.25.1

Re: [PATCH 2/2] exfat: unify name extraction

[Tetsuhiro Kohada]

Thanks for your reply.

On 2020/08/09 2:19, Sungjong Seo wrote:
> [snip]
>> @@ -963,80 +942,38 @@ int exfat_find_dir_entry(struct super_block *sb,
>> struct exfat_inode_info *ei,
>>   			num_empty = 0;
>>   			candi_empty.eidx = EXFAT_HINT_NONE;
>>
> [snip]
>>
>> -			if (entry_type &
>> -					(TYPE_CRITICAL_SEC |
> TYPE_BENIGN_SEC)) {
>> -				if (step == DIRENT_STEP_SECD) {
>> -					if (++order == num_ext)
>> -						goto found;
>> -					continue;
>> -				}
>> +			exfat_get_uniname_from_name_entries(es, &uni_name);
> 
> It is needed to check a return value.

I'll fix it in v2.


>> +			exfat_free_dentry_set(es, false);
>> +
>> +			if (!exfat_uniname_ncmp(sb,
>> +						p_uniname->name,
>> +						uni_name.name,
>> +						name_len)) {
>> +				/* set the last used position as hint */
>> +				hint_stat->clu = clu.dir;
>> +				hint_stat->eidx = dentry;
> 
> eidx and clu of hint_stat should have one for the next entry we'll start
> looking for.
> Did you intentionally change the concept?

Yes, this is intentional.
Essentially, the "Hint" concept is to reduce the next seek cost with minimal cost.
There is a difference in the position of the hint, but the concept is the same.
As you can see, the patched code strategy doesn't move from current position.
Basically, the original code strategy is advancing only one dentry.(It's the "minimum cost")
However, when it reaches the cluster boundary, it gets the next cluster and error handling.
Getting the next cluster The error handling already exists at the end of the while loop,
so the code is duplicated.
These costs should be paid next time and are no longer the "minimum cost".

Should I add this to the commit-message?


BR
---
Tetsuhiro Kohada <kohada.t2@gmail.com>

RE: [PATCH 2/2] exfat: unify name extraction

[Sungjong Seo]

> Thanks for your reply.
> 
> On 2020/08/09 2:19, Sungjong Seo wrote:
> > [snip]
> >> @@ -963,80 +942,38 @@ int exfat_find_dir_entry(struct super_block
> >> *sb, struct exfat_inode_info *ei,
> >>   			num_empty = 0;
> >>   			candi_empty.eidx = EXFAT_HINT_NONE;
> >>
> > [snip]
> >>
> >> -			if (entry_type &
> >> -					(TYPE_CRITICAL_SEC |
> > TYPE_BENIGN_SEC)) {
> >> -				if (step == DIRENT_STEP_SECD) {
> >> -					if (++order == num_ext)
> >> -						goto found;
> >> -					continue;
> >> -				}
> >> +			exfat_get_uniname_from_name_entries(es, &uni_name);
> >
> > It is needed to check a return value.
> 
> I'll fix it in v2.
> 
> 
> >> +			exfat_free_dentry_set(es, false);
> >> +
> >> +			if (!exfat_uniname_ncmp(sb,
> >> +						p_uniname->name,
> >> +						uni_name.name,
> >> +						name_len)) {
> >> +				/* set the last used position as hint */
> >> +				hint_stat->clu = clu.dir;
> >> +				hint_stat->eidx = dentry;
> >
> > eidx and clu of hint_stat should have one for the next entry we'll
> > start looking for.
> > Did you intentionally change the concept?
> 
> Yes, this is intentional.
> Essentially, the "Hint" concept is to reduce the next seek cost with
> minimal cost.
> There is a difference in the position of the hint, but the concept is the
> same.
> As you can see, the patched code strategy doesn't move from current
> position.
> Basically, the original code strategy is advancing only one dentry.(It's
> the "minimum cost") However, when it reaches the cluster boundary, it gets
> the next cluster and error handling.

I didn't get exactly what "original code" is.
Do you mean whole code lines for exfat_find_dir_entry()?
Or just only for handling the hint in it?

The strategy of original code for hint is advancing not one dentry but one dentry_set.
If a hint position is not moved to next like the patched code,
caller have to start at old dentry_set that could be already loaded on dentry cache.

Let's think the case of searching through all files sequentially.
The patched code should check twice per a file.
No better than the original policy.

> Getting the next cluster The error handling already exists at the end of
> the while loop, so the code is duplicated.
> These costs should be paid next time and are no longer the "minimum cost".

I agree with your words, "These costs should be paid next time".
If so, how about moving the cluster handling for a hint dentry to
the beginning of the function while keeping the original policy?

BTW, this patch is not related to the hint code.
I think it would be better to keep the original code in this patch and improve it with a separate patch.

> Should I add this to the commit-message?
> 
> 
> BR
> ---
> Tetsuhiro Kohada <kohada.t2@gmail.com>

Re: [PATCH 2/2] exfat: unify name extraction

[Tetsuhiro Kohada]

>>>> +			exfat_free_dentry_set(es, false);
>>>> +
>>>> +			if (!exfat_uniname_ncmp(sb,
>>>> +						p_uniname->name,
>>>> +						uni_name.name,
>>>> +						name_len)) {
>>>> +				/* set the last used position as hint */
>>>> +				hint_stat->clu = clu.dir;
>>>> +				hint_stat->eidx = dentry;
>>>
>>> eidx and clu of hint_stat should have one for the next entry we'll
>>> start looking for.
>>> Did you intentionally change the concept?
>>
>> Yes, this is intentional.
>> Essentially, the "Hint" concept is to reduce the next seek cost with
>> minimal cost.
>> There is a difference in the position of the hint, but the concept is the
>> same.
>> As you can see, the patched code strategy doesn't move from current
>> position.
>> Basically, the original code strategy is advancing only one dentry.(It's
>> the "minimum cost") However, when it reaches the cluster boundary, it gets
>> the next cluster and error handling.
> 
> I didn't get exactly what "original code" is.
> Do you mean whole code lines for exfat_find_dir_entry()?
> Or just only for handling the hint in it?

My intention is the latter.


> The strategy of original code for hint is advancing not one dentry but one dentry_set.

That's the strategy as a whole code.
But all it does to get a hint after "found" is to advance one entry.
In the original code, the 'dentry' variable points to the end of the EntrySet when "found",
so it can point to the next EntrySet by simply advancing one entry.
(However, it may need to scan the cluster chain)


> If a hint position is not moved to next like the patched code,
> caller have to start at old dentry_set that could be already loaded on dentry cache.
> 
> Let's think the case of searching through all files sequentially.
> The patched code should check twice per a file.

This is the case when all requests find the specified file, right?

Sure, the request will evaluate the same EntrySet as before found.
However, the cost to spend is different from the last time.
The current request looks for a different name than the last request.
In most cases, length and hash are different from the last EntrySet.
Therefore, the last EntrySet just skips dir-entries by num_ext.
There is no string comparison with ignores cases. <- This cost is high
The cost of skipping dir-entries is much less than the string comparison.

> No better than the original policy.

In this patch, when "found", the 'dentry' variable still points to the beginning of the EntrySet.
In this case, I thought "stay here" was a very efficient hint at a minimal cost.
As a whole, I think that the cost has been reduced...
> 
>> Getting the next cluster The error handling already exists at the end of
>> the while loop, so the code is duplicated.
>> These costs should be paid next time and are no longer the "minimum cost".
> 
> I agree with your words, "These costs should be paid next time".
> If so, how about moving the cluster handling for a hint dentry to
> the beginning of the function while keeping the original policy?

My first idea was
	hint_stat->eidx = dentry + 1 + num_ext;

However, in the current hint, offset ((hint_stat->eidx) and cluster number (hint_stat->clu) in the directory are paired.
It was difficult to change only one of values.
So I'm trying to make a 'new hint' where the offset and cluster number aren't linked.


> BTW, this patch is not related to the hint code.
> I think it would be better to keep the original code in this patch and improve it with a separate patch.

I think so, too.
I'll try another patch.


BR
---
Tetsuhiro Kohada <kohada.t2@gmail.com>
> 

RE: [PATCH 1/2] exfat: add NameLength check when extracting name

[Sungjong Seo]

> The current implementation doesn't care NameLength when extracting the
> name from Name dir-entries, so the name may be incorrect.
> (Without null-termination, Insufficient Name dir-entry, etc) Add a
> NameLength check when extracting the name from Name dir-entries to extract
> correct name.
> And, change to get the information of file/stream-ext dir-entries via the
> member variable of exfat_entry_set_cache.
> 
> ** This patch depends on:
>   '[PATCH v3] exfat: integrates dir-entry getting and validation'.
> 
> Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com>
> ---
>  fs/exfat/dir.c | 81 ++++++++++++++++++++++++--------------------------
>  1 file changed, 39 insertions(+), 42 deletions(-)
> 
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
> 91cdbede0fd1..545bb73b95e9 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -28,16 +28,15 @@ static int exfat_extract_uni_name(struct exfat_dentry
> *ep,
> 
>  }
> 
> -static void exfat_get_uniname_from_ext_entry(struct super_block *sb,
> -		struct exfat_chain *p_dir, int entry, unsigned short
> *uniname)
> +static int exfat_get_uniname_from_name_entries(struct
> exfat_entry_set_cache *es,
> +		struct exfat_uni_name *uniname)
>  {
> -	int i;
> -	struct exfat_entry_set_cache *es;
> +	int n, l, i;
>  	struct exfat_dentry *ep;
> 
> -	es = exfat_get_dentry_set(sb, p_dir, entry, ES_ALL_ENTRIES);
> -	if (!es)
> -		return;
> +	uniname->name_len = es->de_stream->name_len;
> +	if (uniname->name_len == 0)
> +		return -EIO;

-EINVAL looks better.

> 
>  	/*
>  	 * First entry  : file entry
> @@ -45,14 +44,15 @@ static void exfat_get_uniname_from_ext_entry(struct
> super_block *sb,
>  	 * Third entry  : first file-name entry
>  	 * So, the index of first file-name dentry should start from 2.
>  	 */
> -
> -	i = 2;
> -	while ((ep = exfat_get_validated_dentry(es, i++, TYPE_NAME))) {
> -		exfat_extract_uni_name(ep, uniname);
> -		uniname += EXFAT_FILE_NAME_LEN;
> +	for (l = 0, n = 2; l < uniname->name_len; n++) {
> +		ep = exfat_get_validated_dentry(es, n, TYPE_NAME);
> +		if (!ep)
> +			return -EIO;
> +		for (i = 0; l < uniname->name_len && i <
EXFAT_FILE_NAME_LEN;
> i++, l++)
> +			uniname->name[l] = le16_to_cpu(ep-
> >dentry.name.unicode_0_14[i]);

Looks good.

>  	}
> -
> -	exfat_free_dentry_set(es, false);
> +	uniname->name[l] = 0;
> +	return 0;
>  }
> 
>  /* read a directory entry from the opened directory */ @@ -63,6 +63,7 @@
> static int exfat_readdir(struct inode *inode, struct exfat_dir_entry
> *dir_entry)
[snip]
> -			*uni_name.name = 0x0;
> -			exfat_get_uniname_from_ext_entry(sb, &dir, dentry,
> -				uni_name.name);
> +			dir_entry->size = le64_to_cpu(es->de_stream-
> >valid_size);
> +
> +			exfat_get_uniname_from_name_entries(es, &uni_name);

Modified function has a return value.
It would be better to check the return value.

>  			exfat_utf16_to_nls(sb, &uni_name,
>  				dir_entry->namebuf.lfn,
>  				dir_entry->namebuf.lfnbuf_len);
> -			brelse(bh);
> 
> -			ep = exfat_get_dentry(sb, &clu, i + 1, &bh, NULL);
> -			if (!ep)
> -				return -EIO;
> -			dir_entry->size =
> -				le64_to_cpu(ep->dentry.stream.valid_size);
> -			brelse(bh);
> +			exfat_free_dentry_set(es, false);
> 
>  			ei->hint_bmap.off = dentry >> dentries_per_clu_bits;
>  			ei->hint_bmap.clu = clu.dir;
> --
> 2.25.1

Re: [PATCH 1/2] exfat: add NameLength check when extracting name

[Tetsuhiro Kohada]

Thanks for your reply.

On 2020/08/09 1:54, Sungjong Seo wrote:
>> The current implementation doesn't care NameLength when extracting the
>> name from Name dir-entries, so the name may be incorrect.
>> (Without null-termination, Insufficient Name dir-entry, etc) Add a
>> NameLength check when extracting the name from Name dir-entries to extract
>> correct name.
>> And, change to get the information of file/stream-ext dir-entries via the
>> member variable of exfat_entry_set_cache.
>>
>> ** This patch depends on:
>>    '[PATCH v3] exfat: integrates dir-entry getting and validation'.
>>
>> Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com>
>> ---
>>   fs/exfat/dir.c | 81 ++++++++++++++++++++++++--------------------------
>>   1 file changed, 39 insertions(+), 42 deletions(-)
>>
>> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
>> 91cdbede0fd1..545bb73b95e9 100644
>> --- a/fs/exfat/dir.c
>> +++ b/fs/exfat/dir.c
>> @@ -28,16 +28,15 @@ static int exfat_extract_uni_name(struct exfat_dentry
>> *ep,
>>
>>   }
>>
>> -static void exfat_get_uniname_from_ext_entry(struct super_block *sb,
>> -		struct exfat_chain *p_dir, int entry, unsigned short
>> *uniname)
>> +static int exfat_get_uniname_from_name_entries(struct
>> exfat_entry_set_cache *es,
>> +		struct exfat_uni_name *uniname)
>>   {
>> -	int i;
>> -	struct exfat_entry_set_cache *es;
>> +	int n, l, i;
>>   	struct exfat_dentry *ep;
>>
>> -	es = exfat_get_dentry_set(sb, p_dir, entry, ES_ALL_ENTRIES);
>> -	if (!es)
>> -		return;
>> +	uniname->name_len = es->de_stream->name_len;
>> +	if (uniname->name_len == 0)
>> +		return -EIO;
> 
> -EINVAL looks better.

OK.
I'll change in v2.

>>   	/*
>>   	 * First entry  : file entry
>> @@ -45,14 +44,15 @@ static void exfat_get_uniname_from_ext_entry(struct
>> super_block *sb,
>>   	 * Third entry  : first file-name entry
>>   	 * So, the index of first file-name dentry should start from 2.
>>   	 */
>> -
>> -	i = 2;
>> -	while ((ep = exfat_get_validated_dentry(es, i++, TYPE_NAME))) {
>> -		exfat_extract_uni_name(ep, uniname);
>> -		uniname += EXFAT_FILE_NAME_LEN;
>> +	for (l = 0, n = 2; l < uniname->name_len; n++) {
>> +		ep = exfat_get_validated_dentry(es, n, TYPE_NAME);
>> +		if (!ep)
>> +			return -EIO;
>> +		for (i = 0; l < uniname->name_len && i <
> EXFAT_FILE_NAME_LEN;
>> i++, l++)
>> +			uniname->name[l] = le16_to_cpu(ep-
>>> dentry.name.unicode_0_14[i]);
> 
> Looks good.
> 
>>   	}
>> -
>> -	exfat_free_dentry_set(es, false);
>> +	uniname->name[l] = 0;
>> +	return 0;
>>   }
>>
>>   /* read a directory entry from the opened directory */ @@ -63,6 +63,7 @@
>> static int exfat_readdir(struct inode *inode, struct exfat_dir_entry
>> *dir_entry)
> [snip]
>> -			*uni_name.name = 0x0;
>> -			exfat_get_uniname_from_ext_entry(sb, &dir, dentry,
>> -				uni_name.name);
>> +			dir_entry->size = le64_to_cpu(es->de_stream-
>>> valid_size);
>> +
>> +			exfat_get_uniname_from_name_entries(es, &uni_name);
> 
> Modified function has a return value.
> It would be better to check the return value.

Oops!
I'll fix it in v2.


>>   			exfat_utf16_to_nls(sb, &uni_name,
>>   				dir_entry->namebuf.lfn,
>>   				dir_entry->namebuf.lfnbuf_len);
>> -			brelse(bh);
>>
>> -			ep = exfat_get_dentry(sb, &clu, i + 1, &bh, NULL);
>> -			if (!ep)
>> -				return -EIO;
>> -			dir_entry->size =
>> -				le64_to_cpu(ep->dentry.stream.valid_size);
>> -			brelse(bh);
>> +			exfat_free_dentry_set(es, false);
>>
>>   			ei->hint_bmap.off = dentry >> dentries_per_clu_bits;
>>   			ei->hint_bmap.clu = clu.dir;
>> --
>> 2.25.1
> 
> 

RE: [PATCH 1/2] exfat: add NameLength check when extracting name

[Namjae Jeon]

> The current implementation doesn't care NameLength when extracting the name from Name dir-entries, so
> the name may be incorrect.
> (Without null-termination, Insufficient Name dir-entry, etc) Add a NameLength check when extracting
> the name from Name dir-entries to extract correct name.
> And, change to get the information of file/stream-ext dir-entries via the member variable of
> exfat_entry_set_cache.
> 
> ** This patch depends on:
>   '[PATCH v3] exfat: integrates dir-entry getting and validation'.
> 
> Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com>
> ---
>  fs/exfat/dir.c | 81 ++++++++++++++++++++++++--------------------------
>  1 file changed, 39 insertions(+), 42 deletions(-)
> 
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index 91cdbede0fd1..545bb73b95e9 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -28,16 +28,15 @@ static int exfat_extract_uni_name(struct exfat_dentry *ep,
> 
>  }
> 
> -static void exfat_get_uniname_from_ext_entry(struct super_block *sb,
> -		struct exfat_chain *p_dir, int entry, unsigned short *uniname)
> +static int exfat_get_uniname_from_name_entries(struct exfat_entry_set_cache *es,
> +		struct exfat_uni_name *uniname)
>  {
> -	int i;
> -	struct exfat_entry_set_cache *es;
> +	int n, l, i;
>  	struct exfat_dentry *ep;
> 
> -	es = exfat_get_dentry_set(sb, p_dir, entry, ES_ALL_ENTRIES);
> -	if (!es)
> -		return;
> +	uniname->name_len = es->de_stream->name_len;
> +	if (uniname->name_len == 0)
> +		return -EIO;
Can we validate ->name_len and name entry ->type in exfat_get_dentry_set() ?
> 
>  	/*
>  	 * First entry  : file entry
> @@ -45,14 +44,15 @@ static void exfat_get_uniname_from_ext_entry(struct super_block *sb,
>  	 * Third entry  : first file-name entry
>  	 * So, the index of first file-name dentry should start from 2.
>  	 */
> -
> -	i = 2;
> -	while ((ep = exfat_get_validated_dentry(es, i++, TYPE_NAME))) {
> -		exfat_extract_uni_name(ep, uniname);
> -		uniname += EXFAT_FILE_NAME_LEN;
> +	for (l = 0, n = 2; l < uniname->name_len; n++) {
> +		ep = exfat_get_validated_dentry(es, n, TYPE_NAME);
> +		if (!ep)
> +			return -EIO;
> +		for (i = 0; l < uniname->name_len && i < EXFAT_FILE_NAME_LEN; i++, l++)
> +			uniname->name[l] = le16_to_cpu(ep->dentry.name.unicode_0_14[i]);
>  	}
> -
> -	exfat_free_dentry_set(es, false);
> +	uniname->name[l] = 0;
> +	return 0;
>  }

Re: [PATCH 1/2] exfat: add NameLength check when extracting name

[Tetsuhiro Kohada]

Thank you for your reply.

>> -static void exfat_get_uniname_from_ext_entry(struct super_block *sb,
>> -		struct exfat_chain *p_dir, int entry, unsigned short *uniname)
>> +static int exfat_get_uniname_from_name_entries(struct exfat_entry_set_cache *es,
>> +		struct exfat_uni_name *uniname)
>>   {
>> -	int i;
>> -	struct exfat_entry_set_cache *es;
>> +	int n, l, i;
>>   	struct exfat_dentry *ep;
>>
>> -	es = exfat_get_dentry_set(sb, p_dir, entry, ES_ALL_ENTRIES);
>> -	if (!es)
>> -		return;
>> +	uniname->name_len = es->de_stream->name_len;
>> +	if (uniname->name_len == 0)
>> +		return -EIO;
> Can we validate ->name_len and name entry ->type in exfat_get_dentry_set() ?

Yes.
As I wrote in a previous email, entry type validation, name-length validation, and name
extraction should not be separated, so implement all of these in exfat_get_dentry_set().
It can be easily implemented by adding uniname to exfat_entry_set_cache and calling
exfat_get_uniname_from_name_entries() from exfat_get_dentry_set().

However, that would be over-implementation.
Not all callers of exfat_get_dentry_set() need a name.
It is enough to validate the name when it is needed.
This is a file-system driver, not fsck.
Validation is possible in exfat_get_dentry_set(), but unnecessary.

Why do you want to validate the name in exfat_get_dentry_set()?


BR
---
Tetsuhiro Kohada <kohada.t2@gmail.com>

RE: [PATCH 1/2] exfat: add NameLength check when extracting name

[Namjae Jeon]

> Thank you for your reply.
> 
> >> -static void exfat_get_uniname_from_ext_entry(struct super_block *sb,
> >> -		struct exfat_chain *p_dir, int entry, unsigned short *uniname)
> >> +static int exfat_get_uniname_from_name_entries(struct exfat_entry_set_cache *es,
> >> +		struct exfat_uni_name *uniname)
> >>   {
> >> -	int i;
> >> -	struct exfat_entry_set_cache *es;
> >> +	int n, l, i;
> >>   	struct exfat_dentry *ep;
> >>
> >> -	es = exfat_get_dentry_set(sb, p_dir, entry, ES_ALL_ENTRIES);
> >> -	if (!es)
> >> -		return;
> >> +	uniname->name_len = es->de_stream->name_len;
> >> +	if (uniname->name_len == 0)
> >> +		return -EIO;
> > Can we validate ->name_len and name entry ->type in exfat_get_dentry_set() ?
> 
> Yes.
> As I wrote in a previous email, entry type validation, name-length validation, and name extraction
> should not be separated, so implement all of these in exfat_get_dentry_set().
> It can be easily implemented by adding uniname to exfat_entry_set_cache and calling
> exfat_get_uniname_from_name_entries() from exfat_get_dentry_set().
No, We can check stream->name_len and name entry type in exfat_get_dentry_set().
And you are already checking entry type with TYPE_SECONDARY in
exfat_get_dentry_set(). Why do we have to check twice?

>
> However, that would be over-implementation.
> Not all callers of exfat_get_dentry_set() need a name.
Where? It will not checked with ES_2_ENTRIES.

> It is enough to validate the name when it is needed.
> This is a file-system driver, not fsck.
Sorry, I don't understand what you are talking about. If there is a problem
in ondisk-metadata, Filesystem should return error.

> Validation is possible in exfat_get_dentry_set(), but unnecessary.
> 
> Why do you want to validate the name in exfat_get_dentry_set()?
exfat_get_dentry_set validates file, stream entry. And you are trying to check
name entries with type_secondary. In addition, trying add the checksum check.
Conversely, Why would you want to add those checks to exfat_get_dentry_set()?
Why do we check only name entries separately? Aren't you intent to return
validated entry set in exfat_get_dentry_set()?
> 
> 
> BR
> ---
> Tetsuhiro Kohada <kohada.t2@gmail.com>

RE: [PATCH 1/2] exfat: add NameLength check when extracting name

[Kohada.Tetsuhiro]

Thank you for your reply.

> > >> -static void exfat_get_uniname_from_ext_entry(struct super_block *sb,
> > >> -		struct exfat_chain *p_dir, int entry, unsigned short *uniname)
> > >> +static int exfat_get_uniname_from_name_entries(struct exfat_entry_set_cache *es,
> > >> +		struct exfat_uni_name *uniname)
> > >>   {
> > >> -	int i;
> > >> -	struct exfat_entry_set_cache *es;
> > >> +	int n, l, i;
> > >>   	struct exfat_dentry *ep;
> > >>
> > >> -	es = exfat_get_dentry_set(sb, p_dir, entry, ES_ALL_ENTRIES);
> > >> -	if (!es)
> > >> -		return;
> > >> +	uniname->name_len = es->de_stream->name_len;
> > >> +	if (uniname->name_len == 0)
> > >> +		return -EIO;
> > > Can we validate ->name_len and name entry ->type in exfat_get_dentry_set() ?
> >
> > Yes.
> > As I wrote in a previous email, entry type validation, name-length
> > validation, and name extraction should not be separated, so implement all of these in exfat_get_dentry_set().
> > It can be easily implemented by adding uniname to
> > exfat_entry_set_cache and calling
> > exfat_get_uniname_from_name_entries() from exfat_get_dentry_set().
> No, We can check stream->name_len and name entry type in exfat_get_dentry_set().

I have no objection to that point.

> And you are already checking entry type with TYPE_SECONDARY in exfat_get_dentry_set(). Why do we have to check twice?

This verification is according to the description in '6.3 Generic Primary DirectoryEntry Template'.
The EntrySet is composed of one primary dir-entry and multiple Secondary dir-entries. 
We can validate the EntrySet by SecondaryCount and SetChecksum recorded in the Primary dir-entry.

The EntrySet checksum validation and dir-entries order validation are according to different descriptions. 
Therefore, it is not a double check.


> > However, that would be over-implementation.
> > Not all callers of exfat_get_dentry_set() need a name.
> Where? It will not checked with ES_2_ENTRIES.

The following functions don't require name.
__exfat_truncate()
__exfat_write_inode()
exfat_map_cluster()
exfat_find()


> > It is enough to validate the name when it is needed.
> > This is a file-system driver, not fsck.
> Sorry, I don't understand what you are talking about. If there is a problem in ondisk-metadata, Filesystem should return
> error.

My explanation may have been inappropriate.
(Verifier is a better metaphor than fsck)
Essentially, the purpose of file-system driver is not to detect inconsistencies.
Of course, FSD should return error when it detects an inconsistency, as you say.
However, I think it is no-need for active inconsistency detection.


> > Validation is possible in exfat_get_dentry_set(), but unnecessary.
> >
> > Why do you want to validate the name in exfat_get_dentry_set()?
> exfat_get_dentry_set validates file, stream entry. 

> And you are trying to check name entries with type_secondary. 

It's a little different.
I'm trying to validate the checksum according to '6.3 Generic Primary DirectoryEntry Template'.

> In addition, trying add the checksum check.
> Conversely, Why would you want to add those checks to exfat_get_dentry_set()?

We should validate the EntrySet before using its contents.
(should not use contents of the EntrySet without validating it)
There are other filesystem designs that include crc/checksum in their each metadata headers.
Such a design can detect inconsistencies easily and effectively.
This verification is especially effective when meta-data is recorded in multiple sectors like the EntrySet.

> Why do we check only name entries separately? 

This verification is according to the description in '7.6.3 NameLength Field' and '7.7 File Name Directory Entry'.
Separated because it is  according to different description from checksum.
As I wrote before, I think TYPE_NAME and NameLength validation should not be separated from the name extraction.
(Because they are more strongly related than order of dir-entries)
So I think these should not be mixed with checksum verification.

When packing names into Name Dir-Entry...
We can also calculate the checksum while packing the name into the Name Dir-Entry.
However, we shouldn't mix them.


> Aren't you intent to return validated entry set in exfat_get_dentry_set()?

If so, add exfat_get_uniname_from_name_entries() after checksum verification.(as below)
----------------------------------------------------
	/* EntrySet checksum verification */

+	if (max_entries == ES_ALL_ENTRIES &&
+	    exfat_get_uniname_from_name_entries(es, es->uniname))
+		goto free_es;
	return es;
----------------------------------------------------

The only callers that need a name are exfat_readdir() and exfat_find_dir_entry(), not the others.
Unnecessary name extraction violates the JIT principle.
(The size of exfat_entry_set_cache will also increase)
And even if we call exfat_get_uniname_from_name_entries() later, we can correctly determine 
"File Name directory entries are valid only if they immediately follow the Stream Extension directory entry as a consecutive series"

File dir-entry set can contain dir-entries other than file, stream-ext and name.
We don't need to validate or extract them all in exfat_get_dentry_set().
I think exfat_entry_set_cache only needs to provide a framework for easy access when needed.

BR
---
Kohada Tetsuhiro <Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp>