* [PATCH v2] exfat: integrates dir-entry getting and validation @ 2020-07-15 1:22 ` Tetsuhiro Kohada 2020-07-29 10:01 ` Tetsuhiro Kohada 2020-07-30 6:53 ` Namjae Jeon 0 siblings, 2 replies; 6+ messages in thread From: Tetsuhiro Kohada @ 2020-07-15 1:22 UTC (permalink / raw) To: kohada.t2 Cc: kohada.tetsuhiro, mori.takahiro, motai.hirotaka, Sungjong Seo, Namjae Jeon, linux-fsdevel, linux-kernel 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 <sj1557.seo@samsung.com> Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com> --- 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] exfat: integrates dir-entry getting and validation 2020-07-15 1:22 ` [PATCH v2] exfat: integrates dir-entry getting and validation Tetsuhiro Kohada @ 2020-07-29 10:01 ` Tetsuhiro Kohada 2020-07-30 6:53 ` Namjae Jeon 1 sibling, 0 replies; 6+ messages in thread From: Tetsuhiro Kohada @ 2020-07-29 10:01 UTC (permalink / raw) To: Sungjong Seo Cc: kohada.tetsuhiro, mori.takahiro, motai.hirotaka, Sungjong Seo, Namjae Jeon, linux-fsdevel, linux-kernel 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 <sj1557.seo@samsung.com> > Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com> > --- > Changes in v2 > - Change verification order > - Verification loop start with index 2 Ping. Is there anything I should do? BR --- Tetsuhiro Kohada <kohada.t2@gmail.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v2] exfat: integrates dir-entry getting and validation 2020-07-15 1:22 ` [PATCH v2] exfat: integrates dir-entry getting and validation Tetsuhiro Kohada 2020-07-29 10:01 ` Tetsuhiro Kohada @ 2020-07-30 6:53 ` Namjae Jeon 2020-08-03 7:31 ` Kohada.Tetsuhiro 1 sibling, 1 reply; 6+ messages in thread From: Namjae Jeon @ 2020-07-30 6:53 UTC (permalink / raw) To: 'Tetsuhiro Kohada' Cc: kohada.tetsuhiro, mori.takahiro, motai.hirotaka, 'Sungjong Seo', linux-fsdevel, linux-kernel > 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 <sj1557.seo@samsung.com> > Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com> > --- > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v2] exfat: integrates dir-entry getting and validation 2020-07-30 6:53 ` Namjae Jeon @ 2020-08-03 7:31 ` Kohada.Tetsuhiro 2020-08-04 1:28 ` Namjae Jeon 0 siblings, 1 reply; 6+ messages in thread From: Kohada.Tetsuhiro @ 2020-08-03 7:31 UTC (permalink / raw) To: 'Namjae Jeon' Cc: Mori.Takahiro, Motai.Hirotaka, 'Sungjong Seo', linux-fsdevel, linux-kernel 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 <Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp> ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v2] exfat: integrates dir-entry getting and validation 2020-08-03 7:31 ` Kohada.Tetsuhiro @ 2020-08-04 1:28 ` Namjae Jeon 2020-08-05 1:44 ` Tetsuhiro Kohada 0 siblings, 1 reply; 6+ messages in thread From: Namjae Jeon @ 2020-08-04 1:28 UTC (permalink / raw) To: Kohada.Tetsuhiro Cc: Mori.Takahiro, Motai.Hirotaka, 'Sungjong Seo', linux-fsdevel, linux-kernel > 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 <Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] exfat: integrates dir-entry getting and validation 2020-08-04 1:28 ` Namjae Jeon @ 2020-08-05 1:44 ` Tetsuhiro Kohada 0 siblings, 0 replies; 6+ messages in thread From: Tetsuhiro Kohada @ 2020-08-05 1:44 UTC (permalink / raw) To: Namjae Jeon, Kohada.Tetsuhiro Cc: Mori.Takahiro, Motai.Hirotaka, 'Sungjong Seo', linux-fsdevel, linux-kernel >>>> + 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 <Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-08-05 1:44 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CGME20200715012304epcas1p23e9f45415afc551beea122e4e1bdb933@epcas1p2.samsung.com> 2020-07-15 1:22 ` [PATCH v2] exfat: integrates dir-entry getting and validation Tetsuhiro Kohada 2020-07-29 10:01 ` Tetsuhiro Kohada 2020-07-30 6:53 ` Namjae Jeon 2020-08-03 7:31 ` Kohada.Tetsuhiro 2020-08-04 1:28 ` Namjae Jeon 2020-08-05 1:44 ` Tetsuhiro Kohada
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).