Add validation for num, bh and type on getting dir-entry.
('file' and 'stream-ext' dir-entries are pre-validated to ensure success)
Renamed exfat_get_dentry_cached() to exfat_get_validated_dentry() due to
a change in functionality.
Integrate type-validation with simplified.
This will also recognize a dir-entry set that contains 'benign secondary'
dir-entries.
And, rename TYPE_EXTEND to TYPE_NAME.
Suggested-by: Sungjong Seo <[email protected]>
Signed-off-by: Tetsuhiro Kohada <[email protected]>
---
Changes in v2
- Change verification order
- Verification loop start with index 2
fs/exfat/dir.c | 144 ++++++++++++++++++--------------------------
fs/exfat/exfat_fs.h | 15 +++--
fs/exfat/file.c | 4 +-
fs/exfat/inode.c | 6 +-
fs/exfat/namei.c | 4 +-
5 files changed, 73 insertions(+), 100 deletions(-)
diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c
index 573659bfbc55..09b85746e760 100644
--- a/fs/exfat/dir.c
+++ b/fs/exfat/dir.c
@@ -33,6 +33,7 @@ static void exfat_get_uniname_from_ext_entry(struct super_block *sb,
{
int i;
struct exfat_entry_set_cache *es;
+ struct exfat_dentry *ep;
es = exfat_get_dentry_set(sb, p_dir, entry, ES_ALL_ENTRIES);
if (!es)
@@ -44,13 +45,9 @@ 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.
*/
- for (i = 2; i < es->num_entries; i++) {
- struct exfat_dentry *ep = exfat_get_dentry_cached(es, i);
-
- /* end of name entry */
- if (exfat_get_entry_type(ep) != TYPE_EXTEND)
- break;
+ i = 2;
+ while ((ep = exfat_get_validated_dentry(es, i++, TYPE_NAME))) {
exfat_extract_uni_name(ep, uniname);
uniname += EXFAT_FILE_NAME_LEN;
}
@@ -372,7 +369,7 @@ unsigned int exfat_get_entry_type(struct exfat_dentry *ep)
if (ep->type == EXFAT_STREAM)
return TYPE_STREAM;
if (ep->type == EXFAT_NAME)
- return TYPE_EXTEND;
+ return TYPE_NAME;
if (ep->type == EXFAT_ACL)
return TYPE_ACL;
return TYPE_CRITICAL_SEC;
@@ -388,7 +385,7 @@ static void exfat_set_entry_type(struct exfat_dentry *ep, unsigned int type)
ep->type &= EXFAT_DELETE;
} else if (type == TYPE_STREAM) {
ep->type = EXFAT_STREAM;
- } else if (type == TYPE_EXTEND) {
+ } else if (type == TYPE_NAME) {
ep->type = EXFAT_NAME;
} else if (type == TYPE_BITMAP) {
ep->type = EXFAT_BITMAP;
@@ -421,7 +418,7 @@ static void exfat_init_name_entry(struct exfat_dentry *ep,
{
int i;
- exfat_set_entry_type(ep, TYPE_EXTEND);
+ exfat_set_entry_type(ep, TYPE_NAME);
ep->dentry.name.flags = 0x0;
for (i = 0; i < EXFAT_FILE_NAME_LEN; i++) {
@@ -594,12 +591,12 @@ void exfat_update_dir_chksum_with_entry_set(struct exfat_entry_set_cache *es)
struct exfat_dentry *ep;
for (i = 0; i < es->num_entries; i++) {
- ep = exfat_get_dentry_cached(es, i);
+ ep = exfat_get_validated_dentry(es, i, TYPE_ALL);
chksum = exfat_calc_chksum16(ep, DENTRY_SIZE, chksum,
chksum_type);
chksum_type = CS_DEFAULT;
}
- ep = exfat_get_dentry_cached(es, 0);
+ ep = exfat_get_validated_dentry(es, 0, TYPE_FILE);
ep->dentry.file.checksum = cpu_to_le16(chksum);
es->modified = true;
}
@@ -741,92 +738,66 @@ struct exfat_dentry *exfat_get_dentry(struct super_block *sb,
return (struct exfat_dentry *)((*bh)->b_data + off);
}
-enum exfat_validate_dentry_mode {
- ES_MODE_STARTED,
- ES_MODE_GET_FILE_ENTRY,
- ES_MODE_GET_STRM_ENTRY,
- ES_MODE_GET_NAME_ENTRY,
- ES_MODE_GET_CRITICAL_SEC_ENTRY,
-};
-
-static bool exfat_validate_entry(unsigned int type,
- enum exfat_validate_dentry_mode *mode)
-{
- if (type == TYPE_UNUSED || type == TYPE_DELETED)
- return false;
-
- switch (*mode) {
- case ES_MODE_STARTED:
- if (type != TYPE_FILE && type != TYPE_DIR)
- return false;
- *mode = ES_MODE_GET_FILE_ENTRY;
- return true;
- case ES_MODE_GET_FILE_ENTRY:
- if (type != TYPE_STREAM)
- return false;
- *mode = ES_MODE_GET_STRM_ENTRY;
- return true;
- case ES_MODE_GET_STRM_ENTRY:
- if (type != TYPE_EXTEND)
- return false;
- *mode = ES_MODE_GET_NAME_ENTRY;
- return true;
- case ES_MODE_GET_NAME_ENTRY:
- if (type == TYPE_STREAM)
- return false;
- if (type != TYPE_EXTEND) {
- if (!(type & TYPE_CRITICAL_SEC))
- return false;
- *mode = ES_MODE_GET_CRITICAL_SEC_ENTRY;
- }
- return true;
- case ES_MODE_GET_CRITICAL_SEC_ENTRY:
- if (type == TYPE_EXTEND || type == TYPE_STREAM)
- return false;
- if ((type & TYPE_CRITICAL_SEC) != TYPE_CRITICAL_SEC)
- return false;
- return true;
- default:
- WARN_ON_ONCE(1);
- return false;
- }
-}
-
-struct exfat_dentry *exfat_get_dentry_cached(
- struct exfat_entry_set_cache *es, int num)
+struct exfat_dentry *exfat_get_validated_dentry(struct exfat_entry_set_cache *es,
+ int num, unsigned int type)
{
int off = es->start_off + num * DENTRY_SIZE;
- struct buffer_head *bh = es->bh[EXFAT_B_TO_BLK(off, es->sb)];
- char *p = bh->b_data + EXFAT_BLK_OFFSET(off, es->sb);
+ struct buffer_head *bh;
+ struct exfat_dentry *ep;
- return (struct exfat_dentry *)p;
+ if (num >= es->num_entries)
+ return NULL;
+
+ bh = es->bh[EXFAT_B_TO_BLK(off, es->sb)];
+ if (!bh)
+ return NULL;
+
+ ep = (struct exfat_dentry *)
+ (bh->b_data + EXFAT_BLK_OFFSET(off, es->sb));
+
+ switch (type) {
+ case TYPE_ALL: /* accept any */
+ break;
+ case TYPE_FILE:
+ if (ep->type != EXFAT_FILE)
+ return NULL;
+ break;
+ case TYPE_SECONDARY:
+ if (!(type & exfat_get_entry_type(ep)))
+ return NULL;
+ break;
+ default:
+ if (type != exfat_get_entry_type(ep))
+ return NULL;
+ }
+ return ep;
}
/*
* Returns a set of dentries for a file or dir.
*
- * Note It provides a direct pointer to bh->data via exfat_get_dentry_cached().
+ * Note It provides a direct pointer to bh->data via exfat_get_validated_dentry().
* User should call exfat_get_dentry_set() after setting 'modified' to apply
* changes made in this entry set to the real device.
*
* in:
* sb+p_dir+entry: indicates a file/dir
- * type: specifies how many dentries should be included.
+ * max_entries: specifies how many dentries should be included.
* return:
* pointer of entry set on success,
* NULL on failure.
+ * note:
+ * On success, guarantee the correct 'file' and 'stream-ext' dir-entries.
*/
struct exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb,
- struct exfat_chain *p_dir, int entry, unsigned int type)
+ struct exfat_chain *p_dir, int entry, int max_entries)
{
int ret, i, num_bh;
- unsigned int off, byte_offset, clu = 0;
+ unsigned int byte_offset, clu = 0;
sector_t sec;
struct exfat_sb_info *sbi = EXFAT_SB(sb);
struct exfat_entry_set_cache *es;
struct exfat_dentry *ep;
- int num_entries;
- enum exfat_validate_dentry_mode mode = ES_MODE_STARTED;
struct buffer_head *bh;
if (p_dir->dir == DIR_DELETED) {
@@ -844,13 +815,13 @@ struct exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb,
return NULL;
es->sb = sb;
es->modified = false;
+ es->num_entries = 1;
/* byte offset in cluster */
byte_offset = EXFAT_CLU_OFFSET(byte_offset, sbi);
/* byte offset in sector */
- off = EXFAT_BLK_OFFSET(byte_offset, sb);
- es->start_off = off;
+ es->start_off = EXFAT_BLK_OFFSET(byte_offset, sb);
/* sector offset in cluster */
sec = EXFAT_B_TO_BLK(byte_offset, sb);
@@ -861,15 +832,12 @@ struct exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb,
goto free_es;
es->bh[es->num_bh++] = bh;
- ep = exfat_get_dentry_cached(es, 0);
- if (!exfat_validate_entry(exfat_get_entry_type(ep), &mode))
+ ep = exfat_get_validated_dentry(es, 0, TYPE_FILE);
+ if (!ep)
goto free_es;
+ es->num_entries = min(ep->dentry.file.num_ext + 1, max_entries);
- num_entries = type == ES_ALL_ENTRIES ?
- ep->dentry.file.num_ext + 1 : type;
- es->num_entries = num_entries;
-
- num_bh = EXFAT_B_TO_BLK_ROUND_UP(off + num_entries * DENTRY_SIZE, sb);
+ num_bh = EXFAT_B_TO_BLK_ROUND_UP(es->start_off + es->num_entries * DENTRY_SIZE, sb);
for (i = 1; i < num_bh; i++) {
/* get the next sector */
if (exfat_is_last_sector_in_cluster(sbi, sec)) {
@@ -889,11 +857,13 @@ struct exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb,
}
/* validiate cached dentries */
- for (i = 1; i < num_entries; i++) {
- ep = exfat_get_dentry_cached(es, i);
- if (!exfat_validate_entry(exfat_get_entry_type(ep), &mode))
+ if (!exfat_get_validated_dentry(es, 1, TYPE_STREAM))
+ goto free_es;
+ for (i = 2; i < es->num_entries; i++) {
+ if (!exfat_get_validated_dentry(es, i, TYPE_SECONDARY))
goto free_es;
}
+
return es;
free_es:
@@ -1028,7 +998,7 @@ int exfat_find_dir_entry(struct super_block *sb, struct exfat_inode_info *ei,
}
brelse(bh);
- if (entry_type == TYPE_EXTEND) {
+ if (entry_type == TYPE_NAME) {
unsigned short entry_uniname[16], unichar;
if (step != DIRENT_STEP_NAME) {
@@ -1144,7 +1114,7 @@ int exfat_count_ext_entries(struct super_block *sb, struct exfat_chain *p_dir,
type = exfat_get_entry_type(ext_ep);
brelse(bh);
- if (type == TYPE_EXTEND || type == TYPE_STREAM)
+ if (type == TYPE_NAME || type == TYPE_STREAM)
count++;
else
break;
diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
index cb51d6e83199..7e07f4645696 100644
--- a/fs/exfat/exfat_fs.h
+++ b/fs/exfat/exfat_fs.h
@@ -40,7 +40,7 @@ enum {
* Type Definitions
*/
#define ES_2_ENTRIES 2
-#define ES_ALL_ENTRIES 0
+#define ES_ALL_ENTRIES 256
#define DIR_DELETED 0xFFFF0321
@@ -56,7 +56,7 @@ enum {
#define TYPE_FILE 0x011F
#define TYPE_CRITICAL_SEC 0x0200
#define TYPE_STREAM 0x0201
-#define TYPE_EXTEND 0x0202
+#define TYPE_NAME 0x0202
#define TYPE_ACL 0x0203
#define TYPE_BENIGN_PRI 0x0400
#define TYPE_GUID 0x0401
@@ -65,6 +65,9 @@ enum {
#define TYPE_BENIGN_SEC 0x0800
#define TYPE_ALL 0x0FFF
+#define TYPE_PRIMARY (TYPE_CRITICAL_PRI | TYPE_BENIGN_PRI)
+#define TYPE_SECONDARY (TYPE_CRITICAL_SEC | TYPE_BENIGN_SEC)
+
#define MAX_CHARSET_SIZE 6 /* max size of multi-byte character */
#define MAX_NAME_LENGTH 255 /* max len of file name excluding NULL */
#define MAX_VFSNAME_BUF_SIZE ((MAX_NAME_LENGTH + 1) * MAX_CHARSET_SIZE)
@@ -171,7 +174,7 @@ struct exfat_entry_set_cache {
unsigned int start_off;
int num_bh;
struct buffer_head *bh[DIR_CACHE_SIZE];
- unsigned int num_entries;
+ int num_entries;
};
struct exfat_dir_entry {
@@ -456,10 +459,10 @@ int exfat_find_location(struct super_block *sb, struct exfat_chain *p_dir,
struct exfat_dentry *exfat_get_dentry(struct super_block *sb,
struct exfat_chain *p_dir, int entry, struct buffer_head **bh,
sector_t *sector);
-struct exfat_dentry *exfat_get_dentry_cached(struct exfat_entry_set_cache *es,
- int num);
+struct exfat_dentry *exfat_get_validated_dentry(struct exfat_entry_set_cache *es,
+ int num, unsigned int type);
struct exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb,
- struct exfat_chain *p_dir, int entry, unsigned int type);
+ struct exfat_chain *p_dir, int entry, int max_entries);
int exfat_free_dentry_set(struct exfat_entry_set_cache *es, int sync);
int exfat_count_dir_entries(struct super_block *sb, struct exfat_chain *p_dir);
diff --git a/fs/exfat/file.c b/fs/exfat/file.c
index 6707f3eb09b5..b6b458e6f5e3 100644
--- a/fs/exfat/file.c
+++ b/fs/exfat/file.c
@@ -160,8 +160,8 @@ int __exfat_truncate(struct inode *inode, loff_t new_size)
ES_ALL_ENTRIES);
if (!es)
return -EIO;
- ep = exfat_get_dentry_cached(es, 0);
- ep2 = exfat_get_dentry_cached(es, 1);
+ ep = exfat_get_validated_dentry(es, 0, TYPE_FILE);
+ ep2 = exfat_get_validated_dentry(es, 1, TYPE_STREAM);
ts = current_time(inode);
exfat_set_entry_time(sbi, &ts,
diff --git a/fs/exfat/inode.c b/fs/exfat/inode.c
index f0160a7892a8..e7bc1ee1761a 100644
--- a/fs/exfat/inode.c
+++ b/fs/exfat/inode.c
@@ -45,8 +45,8 @@ static int __exfat_write_inode(struct inode *inode, int sync)
es = exfat_get_dentry_set(sb, &(ei->dir), ei->entry, ES_ALL_ENTRIES);
if (!es)
return -EIO;
- ep = exfat_get_dentry_cached(es, 0);
- ep2 = exfat_get_dentry_cached(es, 1);
+ ep = exfat_get_validated_dentry(es, 0, TYPE_FILE);
+ ep2 = exfat_get_validated_dentry(es, 1, TYPE_STREAM);
ep->dentry.file.attr = cpu_to_le16(exfat_make_attr(inode));
@@ -228,7 +228,7 @@ static int exfat_map_cluster(struct inode *inode, unsigned int clu_offset,
if (!es)
return -EIO;
/* get stream entry */
- ep = exfat_get_dentry_cached(es, 1);
+ ep = exfat_get_validated_dentry(es, 1, TYPE_STREAM);
/* update directory entry */
ep->dentry.stream.flags = ei->flags;
diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c
index 126ed3ba8f47..47fef6b75f28 100644
--- a/fs/exfat/namei.c
+++ b/fs/exfat/namei.c
@@ -664,8 +664,8 @@ static int exfat_find(struct inode *dir, struct qstr *qname,
es = exfat_get_dentry_set(sb, &cdir, dentry, ES_2_ENTRIES);
if (!es)
return -EIO;
- ep = exfat_get_dentry_cached(es, 0);
- ep2 = exfat_get_dentry_cached(es, 1);
+ ep = exfat_get_validated_dentry(es, 0, TYPE_FILE);
+ ep2 = exfat_get_validated_dentry(es, 1, TYPE_STREAM);
info->type = exfat_get_entry_type(ep);
info->attr = le16_to_cpu(ep->dentry.file.attr);
--
2.25.1
On 2020/07/15 10:22, Tetsuhiro Kohada wrote:
> Add validation for num, bh and type on getting dir-entry.
> ('file' and 'stream-ext' dir-entries are pre-validated to ensure success)
> Renamed exfat_get_dentry_cached() to exfat_get_validated_dentry() due to
> a change in functionality.
>
> Integrate type-validation with simplified.
> This will also recognize a dir-entry set that contains 'benign secondary'
> dir-entries.
>
> And, rename TYPE_EXTEND to TYPE_NAME.
>
> Suggested-by: Sungjong Seo <[email protected]>
> Signed-off-by: Tetsuhiro Kohada <[email protected]>
> ---
> Changes in v2
> - Change verification order
> - Verification loop start with index 2
Ping.
Is there anything I should do?
BR
---
Tetsuhiro Kohada <[email protected]>
> Add validation for num, bh and type on getting dir-entry.
> ('file' and 'stream-ext' dir-entries are pre-validated to ensure success) Renamed
> exfat_get_dentry_cached() to exfat_get_validated_dentry() due to a change in functionality.
>
> Integrate type-validation with simplified.
> This will also recognize a dir-entry set that contains 'benign secondary'
> dir-entries.
>
> And, rename TYPE_EXTEND to TYPE_NAME.
>
> Suggested-by: Sungjong Seo <[email protected]>
> Signed-off-by: Tetsuhiro Kohada <[email protected]>
> ---
> Changes in v2
> - Change verification order
> - Verification loop start with index 2
>
> fs/exfat/dir.c | 144 ++++++++++++++++++--------------------------
> fs/exfat/exfat_fs.h | 15 +++--
> fs/exfat/file.c | 4 +-
> fs/exfat/inode.c | 6 +-
> fs/exfat/namei.c | 4 +-
> 5 files changed, 73 insertions(+), 100 deletions(-)
>
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index 573659bfbc55..09b85746e760 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -33,6 +33,7 @@ static void exfat_get_uniname_from_ext_entry(struct super_block *sb, {
> int i;
> struct exfat_entry_set_cache *es;
> + struct exfat_dentry *ep;
>
> es = exfat_get_dentry_set(sb, p_dir, entry, ES_ALL_ENTRIES);
> if (!es)
> @@ -44,13 +45,9 @@ 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.
> */
> - for (i = 2; i < es->num_entries; i++) {
> - struct exfat_dentry *ep = exfat_get_dentry_cached(es, i);
> -
> - /* end of name entry */
> - if (exfat_get_entry_type(ep) != TYPE_EXTEND)
> - break;
>
> + i = 2;
> + while ((ep = exfat_get_validated_dentry(es, i++, TYPE_NAME))) {
As Sungjong said, I think that TYPE_NAME seems right to be validated in exfat_get_dentry_set().
> exfat_extract_uni_name(ep, uniname);
> uniname += EXFAT_FILE_NAME_LEN;
> }
> @@ -372,7 +369,7 @@ unsigned int exfat_get_entry_type(struct exfat_dentry *ep)
> if (ep->type == EXFAT_STREAM)
> return TYPE_STREAM;
> if (ep->type == EXFAT_NAME)
> - return TYPE_EXTEND;
> + return TYPE_NAME;
> if (ep->type == EXFAT_ACL)
> return TYPE_ACL;
> return TYPE_CRITICAL_SEC;
> @@ -388,7 +385,7 @@ static void exfat_set_entry_type(struct exfat_dentry *ep, unsigned int type)
> ep->type &= EXFAT_DELETE;
> } else if (type == TYPE_STREAM) {
> ep->type = EXFAT_STREAM;
> - } else if (type == TYPE_EXTEND) {
> + } else if (type == TYPE_NAME) {
> ep->type = EXFAT_NAME;
> } else if (type == TYPE_BITMAP) {
> ep->type = EXFAT_BITMAP;
> @@ -421,7 +418,7 @@ static void exfat_init_name_entry(struct exfat_dentry *ep, {
> int i;
>
> - exfat_set_entry_type(ep, TYPE_EXTEND);
> + exfat_set_entry_type(ep, TYPE_NAME);
> ep->dentry.name.flags = 0x0;
>
> for (i = 0; i < EXFAT_FILE_NAME_LEN; i++) { @@ -594,12 +591,12 @@ void
> exfat_update_dir_chksum_with_entry_set(struct exfat_entry_set_cache *es)
> struct exfat_dentry *ep;
>
> for (i = 0; i < es->num_entries; i++) {
> - ep = exfat_get_dentry_cached(es, i);
> + ep = exfat_get_validated_dentry(es, i, TYPE_ALL);
> chksum = exfat_calc_chksum16(ep, DENTRY_SIZE, chksum,
> chksum_type);
> chksum_type = CS_DEFAULT;
> }
> - ep = exfat_get_dentry_cached(es, 0);
> + ep = exfat_get_validated_dentry(es, 0, TYPE_FILE);
> ep->dentry.file.checksum = cpu_to_le16(chksum);
> es->modified = true;
> }
> @@ -741,92 +738,66 @@ struct exfat_dentry *exfat_get_dentry(struct super_block *sb,
> return (struct exfat_dentry *)((*bh)->b_data + off); }
>
> -enum exfat_validate_dentry_mode {
> - ES_MODE_STARTED,
> - ES_MODE_GET_FILE_ENTRY,
> - ES_MODE_GET_STRM_ENTRY,
> - ES_MODE_GET_NAME_ENTRY,
> - ES_MODE_GET_CRITICAL_SEC_ENTRY,
> -};
> -
> -static bool exfat_validate_entry(unsigned int type,
> - enum exfat_validate_dentry_mode *mode)
> -{
> - if (type == TYPE_UNUSED || type == TYPE_DELETED)
> - return false;
> -
> - switch (*mode) {
> - case ES_MODE_STARTED:
> - if (type != TYPE_FILE && type != TYPE_DIR)
> - return false;
> - *mode = ES_MODE_GET_FILE_ENTRY;
> - return true;
> - case ES_MODE_GET_FILE_ENTRY:
> - if (type != TYPE_STREAM)
> - return false;
> - *mode = ES_MODE_GET_STRM_ENTRY;
> - return true;
> - case ES_MODE_GET_STRM_ENTRY:
> - if (type != TYPE_EXTEND)
> - return false;
> - *mode = ES_MODE_GET_NAME_ENTRY;
> - return true;
> - case ES_MODE_GET_NAME_ENTRY:
> - if (type == TYPE_STREAM)
> - return false;
> - if (type != TYPE_EXTEND) {
> - if (!(type & TYPE_CRITICAL_SEC))
> - return false;
> - *mode = ES_MODE_GET_CRITICAL_SEC_ENTRY;
> - }
> - return true;
> - case ES_MODE_GET_CRITICAL_SEC_ENTRY:
> - if (type == TYPE_EXTEND || type == TYPE_STREAM)
> - return false;
> - if ((type & TYPE_CRITICAL_SEC) != TYPE_CRITICAL_SEC)
> - return false;
> - return true;
> - default:
> - WARN_ON_ONCE(1);
> - return false;
> - }
> -}
> -
> -struct exfat_dentry *exfat_get_dentry_cached(
> - struct exfat_entry_set_cache *es, int num)
> +struct exfat_dentry *exfat_get_validated_dentry(struct exfat_entry_set_cache *es,
> + int num, unsigned int type)
Please use two tabs.
> {
> int off = es->start_off + num * DENTRY_SIZE;
> - struct buffer_head *bh = es->bh[EXFAT_B_TO_BLK(off, es->sb)];
> - char *p = bh->b_data + EXFAT_BLK_OFFSET(off, es->sb);
> + struct buffer_head *bh;
> + struct exfat_dentry *ep;
>
> - return (struct exfat_dentry *)p;
> + if (num >= es->num_entries)
> + return NULL;
> +
> + bh = es->bh[EXFAT_B_TO_BLK(off, es->sb)];
> + if (!bh)
> + return NULL;
> +
> + ep = (struct exfat_dentry *)
> + (bh->b_data + EXFAT_BLK_OFFSET(off, es->sb));
> +
> + switch (type) {
> + case TYPE_ALL: /* accept any */
> + break;
> + case TYPE_FILE:
> + if (ep->type != EXFAT_FILE)
> + return NULL;
> + break;
> + case TYPE_SECONDARY:
> + if (!(type & exfat_get_entry_type(ep)))
> + return NULL;
> + break;
Type check should be in this order : FILE->STREAM->NAME->{CRITICAL_SEC|BENIGN_SEC}
I think that you are missing TYPE_NAME check here.
> + default:
> + if (type != exfat_get_entry_type(ep))
> + return NULL;
> + }
> + return ep;
> }
>
> /*
> * Returns a set of dentries for a file or dir.
> *
> - * Note It provides a direct pointer to bh->data via exfat_get_dentry_cached().
> + * Note It provides a direct pointer to bh->data via exfat_get_validated_dentry().
> * User should call exfat_get_dentry_set() after setting 'modified' to apply
> * changes made in this entry set to the real device.
> *
> * in:
> * sb+p_dir+entry: indicates a file/dir
> - * type: specifies how many dentries should be included.
> + * max_entries: specifies how many dentries should be included.
> * return:
> * pointer of entry set on success,
> * NULL on failure.
> + * note:
> + * On success, guarantee the correct 'file' and 'stream-ext' dir-entries.
This comment seems unnecessary.
> */
> struct exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb,
> - struct exfat_chain *p_dir, int entry, unsigned int type)
> + struct exfat_chain *p_dir, int entry, int max_entries)
> {
> int ret, i, num_bh;
> - unsigned int off, byte_offset, clu = 0;
> + unsigned int byte_offset, clu = 0;
> sector_t sec;
> struct exfat_sb_info *sbi = EXFAT_SB(sb);
> struct exfat_entry_set_cache *es;
> struct exfat_dentry *ep;
> - int num_entries;
> - enum exfat_validate_dentry_mode mode = ES_MODE_STARTED;
> struct buffer_head *bh;
>
> if (p_dir->dir == DIR_DELETED) {
> @@ -844,13 +815,13 @@ struct exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb,
> return NULL;
> es->sb = sb;
> es->modified = false;
> + es->num_entries = 1;
>
> /* byte offset in cluster */
> byte_offset = EXFAT_CLU_OFFSET(byte_offset, sbi);
>
> /* byte offset in sector */
> - off = EXFAT_BLK_OFFSET(byte_offset, sb);
> - es->start_off = off;
> + es->start_off = EXFAT_BLK_OFFSET(byte_offset, sb);
>
> /* sector offset in cluster */
> sec = EXFAT_B_TO_BLK(byte_offset, sb); @@ -861,15 +832,12 @@ struct exfat_entry_set_cache
> *exfat_get_dentry_set(struct super_block *sb,
> goto free_es;
> es->bh[es->num_bh++] = bh;
>
> - ep = exfat_get_dentry_cached(es, 0);
> - if (!exfat_validate_entry(exfat_get_entry_type(ep), &mode))
> + ep = exfat_get_validated_dentry(es, 0, TYPE_FILE);
> + if (!ep)
> goto free_es;
> + es->num_entries = min(ep->dentry.file.num_ext + 1, max_entries);
>
> - num_entries = type == ES_ALL_ENTRIES ?
> - ep->dentry.file.num_ext + 1 : type;
> - es->num_entries = num_entries;
> -
> - num_bh = EXFAT_B_TO_BLK_ROUND_UP(off + num_entries * DENTRY_SIZE, sb);
> + num_bh = EXFAT_B_TO_BLK_ROUND_UP(es->start_off + es->num_entries *
> +DENTRY_SIZE, sb);
> for (i = 1; i < num_bh; i++) {
> /* get the next sector */
> if (exfat_is_last_sector_in_cluster(sbi, sec)) { @@ -889,11 +857,13 @@ struct
> exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb,
> }
>
> /* validiate cached dentries */
> - for (i = 1; i < num_entries; i++) {
> - ep = exfat_get_dentry_cached(es, i);
> - if (!exfat_validate_entry(exfat_get_entry_type(ep), &mode))
> + if (!exfat_get_validated_dentry(es, 1, TYPE_STREAM))
> + goto free_es;
> + for (i = 2; i < es->num_entries; i++) {
> + if (!exfat_get_validated_dentry(es, i, TYPE_SECONDARY))
> goto free_es;
> }
> +
> return es;
>
> free_es:
> @@ -1028,7 +998,7 @@ int exfat_find_dir_entry(struct super_block *sb, struct exfat_inode_info *ei,
> }
>
> brelse(bh);
> - if (entry_type == TYPE_EXTEND) {
> + if (entry_type == TYPE_NAME) {
> unsigned short entry_uniname[16], unichar;
>
> if (step != DIRENT_STEP_NAME) {
> @@ -1144,7 +1114,7 @@ int exfat_count_ext_entries(struct super_block *sb, struct exfat_chain *p_dir,
>
> type = exfat_get_entry_type(ext_ep);
> brelse(bh);
> - if (type == TYPE_EXTEND || type == TYPE_STREAM)
> + if (type == TYPE_NAME || type == TYPE_STREAM)
> count++;
> else
> break;
> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h index cb51d6e83199..7e07f4645696 100644
> --- a/fs/exfat/exfat_fs.h
> +++ b/fs/exfat/exfat_fs.h
> @@ -40,7 +40,7 @@ enum {
> * Type Definitions
> */
> #define ES_2_ENTRIES 2
> -#define ES_ALL_ENTRIES 0
> +#define ES_ALL_ENTRIES 256
>
> #define DIR_DELETED 0xFFFF0321
>
> @@ -56,7 +56,7 @@ enum {
> #define TYPE_FILE 0x011F
> #define TYPE_CRITICAL_SEC 0x0200
> #define TYPE_STREAM 0x0201
> -#define TYPE_EXTEND 0x0202
> +#define TYPE_NAME 0x0202
> #define TYPE_ACL 0x0203
> #define TYPE_BENIGN_PRI 0x0400
> #define TYPE_GUID 0x0401
> @@ -65,6 +65,9 @@ enum {
> #define TYPE_BENIGN_SEC 0x0800
> #define TYPE_ALL 0x0FFF
>
> +#define TYPE_PRIMARY (TYPE_CRITICAL_PRI | TYPE_BENIGN_PRI)
> +#define TYPE_SECONDARY (TYPE_CRITICAL_SEC | TYPE_BENIGN_SEC)
> +
> #define MAX_CHARSET_SIZE 6 /* max size of multi-byte character */
> #define MAX_NAME_LENGTH 255 /* max len of file name excluding NULL */
> #define MAX_VFSNAME_BUF_SIZE ((MAX_NAME_LENGTH + 1) * MAX_CHARSET_SIZE)
> @@ -171,7 +174,7 @@ struct exfat_entry_set_cache {
> unsigned int start_off;
> int num_bh;
> struct buffer_head *bh[DIR_CACHE_SIZE];
> - unsigned int num_entries;
> + int num_entries;
> };
>
> struct exfat_dir_entry {
> @@ -456,10 +459,10 @@ int exfat_find_location(struct super_block *sb, struct exfat_chain *p_dir,
> struct exfat_dentry *exfat_get_dentry(struct super_block *sb,
> struct exfat_chain *p_dir, int entry, struct buffer_head **bh,
> sector_t *sector);
> -struct exfat_dentry *exfat_get_dentry_cached(struct exfat_entry_set_cache *es,
> - int num);
> +struct exfat_dentry *exfat_get_validated_dentry(struct exfat_entry_set_cache *es,
> + int num, unsigned int type);
> struct exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb,
> - struct exfat_chain *p_dir, int entry, unsigned int type);
> + struct exfat_chain *p_dir, int entry, int max_entries);
> int exfat_free_dentry_set(struct exfat_entry_set_cache *es, int sync); int
> exfat_count_dir_entries(struct super_block *sb, struct exfat_chain *p_dir);
>
> diff --git a/fs/exfat/file.c b/fs/exfat/file.c index 6707f3eb09b5..b6b458e6f5e3 100644
> --- a/fs/exfat/file.c
> +++ b/fs/exfat/file.c
> @@ -160,8 +160,8 @@ int __exfat_truncate(struct inode *inode, loff_t new_size)
> ES_ALL_ENTRIES);
> if (!es)
> return -EIO;
> - ep = exfat_get_dentry_cached(es, 0);
> - ep2 = exfat_get_dentry_cached(es, 1);
> + ep = exfat_get_validated_dentry(es, 0, TYPE_FILE);
> + ep2 = exfat_get_validated_dentry(es, 1, TYPE_STREAM);
TYPE_FILE and TYPE_STREAM was already validated in exfat_get_dentry_set().
Isn't it unnecessary duplication check ?
>
> ts = current_time(inode);
> exfat_set_entry_time(sbi, &ts,
> diff --git a/fs/exfat/inode.c b/fs/exfat/inode.c index f0160a7892a8..e7bc1ee1761a 100644
> --- a/fs/exfat/inode.c
> +++ b/fs/exfat/inode.c
> @@ -45,8 +45,8 @@ static int __exfat_write_inode(struct inode *inode, int sync)
> es = exfat_get_dentry_set(sb, &(ei->dir), ei->entry, ES_ALL_ENTRIES);
> if (!es)
> return -EIO;
> - ep = exfat_get_dentry_cached(es, 0);
> - ep2 = exfat_get_dentry_cached(es, 1);
> + ep = exfat_get_validated_dentry(es, 0, TYPE_FILE);
> + ep2 = exfat_get_validated_dentry(es, 1, TYPE_STREAM);
Ditto.
>
> ep->dentry.file.attr = cpu_to_le16(exfat_make_attr(inode));
>
> @@ -228,7 +228,7 @@ static int exfat_map_cluster(struct inode *inode, unsigned int clu_offset,
> if (!es)
> return -EIO;
> /* get stream entry */
> - ep = exfat_get_dentry_cached(es, 1);
> + ep = exfat_get_validated_dentry(es, 1, TYPE_STREAM);
>
> /* update directory entry */
> ep->dentry.stream.flags = ei->flags; diff --git a/fs/exfat/namei.c
> b/fs/exfat/namei.c index 126ed3ba8f47..47fef6b75f28 100644
> --- a/fs/exfat/namei.c
> +++ b/fs/exfat/namei.c
> @@ -664,8 +664,8 @@ static int exfat_find(struct inode *dir, struct qstr *qname,
> es = exfat_get_dentry_set(sb, &cdir, dentry, ES_2_ENTRIES);
> if (!es)
> return -EIO;
> - ep = exfat_get_dentry_cached(es, 0);
> - ep2 = exfat_get_dentry_cached(es, 1);
> + ep = exfat_get_validated_dentry(es, 0, TYPE_FILE);
> + ep2 = exfat_get_validated_dentry(es, 1, TYPE_STREAM);
Ditto.
>
> info->type = exfat_get_entry_type(ep);
> info->attr = le16_to_cpu(ep->dentry.file.attr);
> --
> 2.25.1
Thank you for your reply.
> > diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
> > 573659bfbc55..09b85746e760 100644
> > --- a/fs/exfat/dir.c
> > +++ b/fs/exfat/dir.c
> > @@ -33,6 +33,7 @@ static void exfat_get_uniname_from_ext_entry(struct super_block *sb, {
> > int i;
> > struct exfat_entry_set_cache *es;
> > + struct exfat_dentry *ep;
> >
> > es = exfat_get_dentry_set(sb, p_dir, entry, ES_ALL_ENTRIES);
> > if (!es)
> > @@ -44,13 +45,9 @@ 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.
> > */
> > - for (i = 2; i < es->num_entries; i++) {
> > - struct exfat_dentry *ep = exfat_get_dentry_cached(es, i);
> > -
> > - /* end of name entry */
> > - if (exfat_get_entry_type(ep) != TYPE_EXTEND)
> > - break;
> >
> > + i = 2;
> > + while ((ep = exfat_get_validated_dentry(es, i++, TYPE_NAME))) {
> As Sungjong said, I think that TYPE_NAME seems right to be validated in exfat_get_dentry_set().
First, it is possible to correctly determine that
"Immediately follow the Stream Extension directory entry as a consecutive series"
whether the TYPE_NAME check is implemented here or exfat_get_dentry_set().
It's functionally same, so it is also right to validate in either.
Second, the current implementation does not care for NameLength field, as I replied to Sungjong.
If name is not terminated with zero, the name will be incorrect.(With or without my patch)
I think TYPE_NAME and NameLength validation should not be separated from the name extraction.
If validate TYPE_NAME in exfat_get_dentry_set(), NameLength validation and name extraction
should also be implemented there.
(Otherwise, a duplication check with exfat_get_dentry_set() and here.)
I will add NameLength validation here.
Therefore, TYPE_NAME validation here should not be omitted.
Third, getting dentry and entry-type validation should be integrated.
These no longer have to be primitive.
The integration simplifies caller error checking.
> > -struct exfat_dentry *exfat_get_dentry_cached(
> > - struct exfat_entry_set_cache *es, int num)
> > +struct exfat_dentry *exfat_get_validated_dentry(struct exfat_entry_set_cache *es,
> > + int num, unsigned int type)
> Please use two tabs.
OK.
I'll fix it.
> > + struct buffer_head *bh;
> > + struct exfat_dentry *ep;
> >
> > - return (struct exfat_dentry *)p;
> > + if (num >= es->num_entries)
> > + return NULL;
> > +
> > + bh = es->bh[EXFAT_B_TO_BLK(off, es->sb)];
> > + if (!bh)
> > + return NULL;
> > +
> > + ep = (struct exfat_dentry *)
> > + (bh->b_data + EXFAT_BLK_OFFSET(off, es->sb));
> > +
> > + switch (type) {
> > + case TYPE_ALL: /* accept any */
> > + break;
> > + case TYPE_FILE:
> > + if (ep->type != EXFAT_FILE)
> > + return NULL;
> > + break;
> > + case TYPE_SECONDARY:
> > + if (!(type & exfat_get_entry_type(ep)))
> > + return NULL;
> > + break;
> Type check should be in this order : FILE->STREAM->NAME->{CRITICAL_SEC|BENIGN_SEC}
> I think that you are missing TYPE_NAME check here.
Types other than the above (TYPE_NAME, TYPE_STREAM, etc.) are checked in the default-case(as below).
> > + default:
> > + if (type != exfat_get_entry_type(ep))
> > + return NULL;
> > + }
> > + return ep;
> > }
> >
> > /*
> > * Returns a set of dentries for a file or dir.
> > *
> > - * Note It provides a direct pointer to bh->data via exfat_get_dentry_cached().
> > + * Note It provides a direct pointer to bh->data via exfat_get_validated_dentry().
> > * User should call exfat_get_dentry_set() after setting 'modified' to apply
> > * changes made in this entry set to the real device.
> > *
> > * in:
> > * sb+p_dir+entry: indicates a file/dir
> > - * type: specifies how many dentries should be included.
> > + * max_entries: specifies how many dentries should be included.
> > * return:
> > * pointer of entry set on success,
> > * NULL on failure.
> > + * note:
> > + * On success, guarantee the correct 'file' and 'stream-ext' dir-entries.
> This comment seems unnecessary.
I'll remove it.
> > diff --git a/fs/exfat/file.c b/fs/exfat/file.c index
> > 6707f3eb09b5..b6b458e6f5e3 100644
> > --- a/fs/exfat/file.c
> > +++ b/fs/exfat/file.c
> > @@ -160,8 +160,8 @@ int __exfat_truncate(struct inode *inode, loff_t new_size)
> > ES_ALL_ENTRIES);
> > if (!es)
> > return -EIO;
> > - ep = exfat_get_dentry_cached(es, 0);
> > - ep2 = exfat_get_dentry_cached(es, 1);
> > + ep = exfat_get_validated_dentry(es, 0, TYPE_FILE);
> > + ep2 = exfat_get_validated_dentry(es, 1, TYPE_STREAM);
> TYPE_FILE and TYPE_STREAM was already validated in exfat_get_dentry_set().
> Isn't it unnecessary duplication check ?
No, as you say.
Although TYPE is specified, it is not good not to check the null of ep/ep2.
However, with TYPE_ALL, it becomes difficult to understand what purpose ep/ep2 is used for.
Therefore, I proposed adding ep_file/ep_stream to es, and here
ep = es->ep_file;
ep2 = es->ep_stream;
How about this?
Or is it better to specify TYPE_ALL?
BTW
It's been about a month since I posted this patch.
In the meantime, I created a NameLength check and a checksum validation based on this patch.
Can you review those as well?
BR
---
Kohada Tetsuhiro <[email protected]>
> Thank you for your reply.
>
> > > diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
> > > 573659bfbc55..09b85746e760 100644
> > > --- a/fs/exfat/dir.c
> > > +++ b/fs/exfat/dir.c
> > > @@ -33,6 +33,7 @@ static void exfat_get_uniname_from_ext_entry(struct super_block *sb, {
> > > int i;
> > > struct exfat_entry_set_cache *es;
> > > + struct exfat_dentry *ep;
> > >
> > > es = exfat_get_dentry_set(sb, p_dir, entry, ES_ALL_ENTRIES);
> > > if (!es)
> > > @@ -44,13 +45,9 @@ 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.
> > > */
> > > - for (i = 2; i < es->num_entries; i++) {
> > > - struct exfat_dentry *ep = exfat_get_dentry_cached(es, i);
> > > -
> > > - /* end of name entry */
> > > - if (exfat_get_entry_type(ep) != TYPE_EXTEND)
> > > - break;
> > >
> > > + i = 2;
> > > + while ((ep = exfat_get_validated_dentry(es, i++, TYPE_NAME))) {
> > As Sungjong said, I think that TYPE_NAME seems right to be validated in exfat_get_dentry_set().
>
> First, it is possible to correctly determine that "Immediately follow the Stream Extension directory
> entry as a consecutive series"
> whether the TYPE_NAME check is implemented here or exfat_get_dentry_set().
> It's functionally same, so it is also right to validate in either.
>
> Second, the current implementation does not care for NameLength field, as I replied to Sungjong.
> If name is not terminated with zero, the name will be incorrect.(With or without my patch) I think
> TYPE_NAME and NameLength validation should not be separated from the name extraction.
> If validate TYPE_NAME in exfat_get_dentry_set(), NameLength validation and name extraction should also
> be implemented there.
> (Otherwise, a duplication check with exfat_get_dentry_set() and here.) I will add NameLength
> validation here.
Okay.
> Therefore, TYPE_NAME validation here should not be omitted.
>
> Third, getting dentry and entry-type validation should be integrated.
> These no longer have to be primitive.
> The integration simplifies caller error checking.
>
>
> > > -struct exfat_dentry *exfat_get_dentry_cached(
> > > - struct exfat_entry_set_cache *es, int num)
> > > +struct exfat_dentry *exfat_get_validated_dentry(struct exfat_entry_set_cache *es,
> > > + int num, unsigned int type)
> > Please use two tabs.
>
> OK.
> I'll fix it.
>
>
> > > + struct buffer_head *bh;
> > > + struct exfat_dentry *ep;
> > >
> > > - return (struct exfat_dentry *)p;
> > > + if (num >= es->num_entries)
> > > + return NULL;
> > > +
> > > + bh = es->bh[EXFAT_B_TO_BLK(off, es->sb)];
> > > + if (!bh)
> > > + return NULL;
> > > +
> > > + ep = (struct exfat_dentry *)
> > > + (bh->b_data + EXFAT_BLK_OFFSET(off, es->sb));
> > > +
> > > + switch (type) {
> > > + case TYPE_ALL: /* accept any */
> > > + break;
> > > + case TYPE_FILE:
> > > + if (ep->type != EXFAT_FILE)
> > > + return NULL;
> > > + break;
> > > + case TYPE_SECONDARY:
> > > + if (!(type & exfat_get_entry_type(ep)))
> > > + return NULL;
> > > + break;
> > Type check should be in this order :
> > FILE->STREAM->NAME->{CRITICAL_SEC|BENIGN_SEC}
> > I think that you are missing TYPE_NAME check here.
>
> Types other than the above (TYPE_NAME, TYPE_STREAM, etc.) are checked in the default-case(as below).
>
> > > + default:
> > > + if (type != exfat_get_entry_type(ep))
> > > + return NULL;
> > > + }
> > > + return ep;
> > > }
> > >
> > > /*
> > > * Returns a set of dentries for a file or dir.
> > > *
> > > - * Note It provides a direct pointer to bh->data via exfat_get_dentry_cached().
> > > + * Note It provides a direct pointer to bh->data via exfat_get_validated_dentry().
> > > * User should call exfat_get_dentry_set() after setting 'modified' to apply
> > > * changes made in this entry set to the real device.
> > > *
> > > * in:
> > > * sb+p_dir+entry: indicates a file/dir
> > > - * type: specifies how many dentries should be included.
> > > + * max_entries: specifies how many dentries should be included.
> > > * return:
> > > * pointer of entry set on success,
> > > * NULL on failure.
> > > + * note:
> > > + * On success, guarantee the correct 'file' and 'stream-ext' dir-entries.
> > This comment seems unnecessary.
>
> I'll remove it.
>
> > > diff --git a/fs/exfat/file.c b/fs/exfat/file.c index
> > > 6707f3eb09b5..b6b458e6f5e3 100644
> > > --- a/fs/exfat/file.c
> > > +++ b/fs/exfat/file.c
> > > @@ -160,8 +160,8 @@ int __exfat_truncate(struct inode *inode, loff_t new_size)
> > > ES_ALL_ENTRIES);
> > > if (!es)
> > > return -EIO;
> > > - ep = exfat_get_dentry_cached(es, 0);
> > > - ep2 = exfat_get_dentry_cached(es, 1);
> > > + ep = exfat_get_validated_dentry(es, 0, TYPE_FILE);
> > > + ep2 = exfat_get_validated_dentry(es, 1, TYPE_STREAM);
> > TYPE_FILE and TYPE_STREAM was already validated in exfat_get_dentry_set().
> > Isn't it unnecessary duplication check ?
>
> No, as you say.
> Although TYPE is specified, it is not good not to check the null of ep/ep2.
> However, with TYPE_ALL, it becomes difficult to understand what purpose ep/ep2 is used for.
> Therefore, I proposed adding ep_file/ep_stream to es, and here
> ep = es->ep_file;
> ep2 = es->ep_stream;
>
> How about this?
You can factor out exfat_get_dentry_cached() from exfat_get_validated_dentry() and use it here.
And then, You can rename ep and ep2 to ep_file and ep_stream.
> Or is it better to specify TYPE_ALL?
>
>
> BTW
> It's been about a month since I posted this patch.
> In the meantime, I created a NameLength check and a checksum validation based on this patch.
> Can you review those as well?
Let me see the patches.
Thanks!
>
> BR
> ---
> Kohada Tetsuhiro <[email protected]>
>>>> + i = 2;
>>>> + while ((ep = exfat_get_validated_dentry(es, i++, TYPE_NAME))) {
>>> As Sungjong said, I think that TYPE_NAME seems right to be validated in exfat_get_dentry_set().
>>
>> First, it is possible to correctly determine that "Immediately follow the Stream Extension directory
>> entry as a consecutive series"
>> whether the TYPE_NAME check is implemented here or exfat_get_dentry_set().
>> It's functionally same, so it is also right to validate in either.
>>
>> Second, the current implementation does not care for NameLength field, as I replied to Sungjong.
>> If name is not terminated with zero, the name will be incorrect.(With or without my patch) I think
>> TYPE_NAME and NameLength validation should not be separated from the name extraction.
>> If validate TYPE_NAME in exfat_get_dentry_set(), NameLength validation and name extraction should also
>> be implemented there.
>> (Otherwise, a duplication check with exfat_get_dentry_set() and here.) I will add NameLength
>> validation here.
> Okay.
Thank you for your understanding.
>> Therefore, TYPE_NAME validation here should not be omitted.
>>
>> Third, getting dentry and entry-type validation should be integrated.
>> These no longer have to be primitive.
>> The integration simplifies caller error checking.
>>>> diff --git a/fs/exfat/file.c b/fs/exfat/file.c index
>>>> 6707f3eb09b5..b6b458e6f5e3 100644
>>>> --- a/fs/exfat/file.c
>>>> +++ b/fs/exfat/file.c
>>>> @@ -160,8 +160,8 @@ int __exfat_truncate(struct inode *inode, loff_t new_size)
>>>> ES_ALL_ENTRIES);
>>>> if (!es)
>>>> return -EIO;
>>>> - ep = exfat_get_dentry_cached(es, 0);
>>>> - ep2 = exfat_get_dentry_cached(es, 1);
>>>> + ep = exfat_get_validated_dentry(es, 0, TYPE_FILE);
>>>> + ep2 = exfat_get_validated_dentry(es, 1, TYPE_STREAM);
>>> TYPE_FILE and TYPE_STREAM was already validated in exfat_get_dentry_set().
>>> Isn't it unnecessary duplication check ?
>>
>> No, as you say.
>> Although TYPE is specified, it is not good not to check the null of ep/ep2.
>> However, with TYPE_ALL, it becomes difficult to understand what purpose ep/ep2 is used for.
>> Therefore, I proposed adding ep_file/ep_stream to es, and here
>> ep = es->ep_file;
>> ep2 = es->ep_stream;
>>
>> How about this?
> You can factor out exfat_get_dentry_cached() from exfat_get_validated_dentry() and use it here.
I actually implemented and use it, but I feel it is not so good.
- Since there are two functions to get from es, so it's a bit confusing which one is better for use.
- There was the same anxiety as using exfat_get_validated_dentry() in that there is no NULL check of
ep got with exfat_get_dentry_cached().
Whichever function I use, there are places where I check the return value and where I don't.
This will cause missing entry-type validation or missing return value check,in the future.
I think it's easier to use by including it as a validated object in the member of exfat_entry_set_cache.
> And then, You can rename ep and ep2 to ep_file and ep_stream.
I propose a slightly different approach than last.
Add members to exfat_entry_set_cache as below.
struct exfat_de_file *de_file;
struct exfat_de_stream *de_stream;
And, use these as below.
es->de_file->attr = cpu_to_le16(exfat_make_attr(inode));
es->de_stream->valid_size = cpu_to_le64(on_disk_size);
exfat_de_file/exfat_de_stream corresponds to the file dir-entry/stream dir-enty structure in the exfat_dentry union.
We can use the validated valid values directly.
Furthermore, these are strongly typed.
>> Or is it better to specify TYPE_ALL?
>>
>>
>> BTW
>> It's been about a month since I posted this patch.
>> In the meantime, I created a NameLength check and a checksum validation based on this patch.
>> Can you review those as well?
> Let me see the patches.
Thanks a lot.
For now, I will create and post a V3 patch with this proposal.
After that, I will recreate the NameLength check and a checksum validation patches based on the V3 patch and post them.
(Should I post these as an RFC?)
BR
---
Kohada Tetsuhiro <[email protected]>