| Message ID | 20200806010229.24690-1-kohada.t2@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] exfat: integrates dir-entry getting and validation | expand |
> Add validation for num, bh and type on getting dir-entry. > 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. > > Pre-Validated 'file' and 'stream-ext' dir-entries are provided as member > variables of exfat_entry_set_cache. > > And, rename TYPE_EXTEND to TYPE_NAME. > > Suggested-by: Sungjong Seo <sj1557.seo@samsung.com> > Suggested-by: Namjae Jeon <namjae.jeon@samsung.com> > Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com> Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com> Looks good to me. Thanks. > --- > Changes in v2 > - Change verification order > - Verification loop start with index 2 > Changes in v3 > - Fix indent > - Fix comment of exfat_get_dentry_set() > - Add de_file/de_stream in exfat_entry_set_cache > - Add srtuct tag name for each dir-entry type in exfat_dentry > - Add description about de_file/de_stream to commit-log > > fs/exfat/dir.c | 147 +++++++++++++++++-------------------------- > fs/exfat/exfat_fs.h | 17 +++-- > fs/exfat/exfat_raw.h | 10 +-- > fs/exfat/file.c | 25 ++++---- > fs/exfat/inode.c | 49 ++++++--------- > fs/exfat/namei.c | 36 +++++------ > 6 files changed, 122 insertions(+), 162 deletions(-)
> > +#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,9 @@ 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_de_file *de_file; > + struct exfat_de_stream *de_stream; I prefer to assign validated entries to **de and use it using enum value. struct exfat_dentry **de; > };
Thank you for your reply. >> @@ -171,7 +174,9 @@ 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_de_file *de_file; >> + struct exfat_de_stream *de_stream; > I prefer to assign validated entries to **de and use it using enum value. > struct exfat_dentry **de; I've tried several implementations that add a struct exfat_dentry type.(*de0 & *de1; *de[2]; etc...) The problem with the struct exfat_dentry type is that it is too flexible for type. This means weak typing. Therefore, when using them, de[XXX_FILE]->dentry.file.zzz ... It is necessary to re-specify the type. (against the DRY principle) Strong typing prevents use with wrong type, at compiling. I think the approach of using de_file/de_stream could be strongly typed. I don't think we need excessive flexibility. BR --- Tetsuhiro Kohada <kohada.t2@gmail.com>
> Thank you for your reply. > > >> @@ -171,7 +174,9 @@ 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_de_file *de_file; > >> + struct exfat_de_stream *de_stream; > > I prefer to assign validated entries to **de and use it using enum value. > > struct exfat_dentry **de; > > I've tried several implementations that add a struct exfat_dentry type.(*de0 & *de1; *de[2]; etc...) > The problem with the struct exfat_dentry type is that it is too flexible for type. > This means weak typing. > Therefore, when using them, > de[XXX_FILE]->dentry.file.zzz ... > It is necessary to re-specify the type. (against the DRY principle) Strong typing prevents use with > wrong type, at compiling. > > I think the approach of using de_file/de_stream could be strongly typed. > I don't think we need excessive flexibility. Could you check the following change ? If you think it's okay, please add it to your patch series. diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index 573659bfbc55..37e0f92f74b3 100644 --- a/fs/exfat/dir.c +++ b/fs/exfat/dir.c @@ -44,14 +44,12 @@ 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); - + for (i = ENTRY_NAME; i < es->num_entries; i++) { /* end of name entry */ - if (exfat_get_entry_type(ep) != TYPE_EXTEND) + if (exfat_get_entry_type(es->de[i]) != TYPE_NAME) break; - exfat_extract_uni_name(ep, uniname); + exfat_extract_uni_name(es->de[i], uniname); uniname += EXFAT_FILE_NAME_LEN; } @@ -372,7 +370,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 +386,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 +419,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++) { @@ -591,16 +589,13 @@ void exfat_update_dir_chksum_with_entry_set(struct exfat_entry_set_cache *es) { int chksum_type = CS_DIR_ENTRY, i; unsigned short chksum = 0; - struct exfat_dentry *ep; for (i = 0; i < es->num_entries; i++) { - ep = exfat_get_dentry_cached(es, i); - chksum = exfat_calc_chksum16(ep, DENTRY_SIZE, chksum, + chksum = exfat_calc_chksum16(es->de[i], DENTRY_SIZE, chksum, chksum_type); chksum_type = CS_DEFAULT; } - ep = exfat_get_dentry_cached(es, 0); - ep->dentry.file.checksum = cpu_to_le16(chksum); + ES_FILE(es).checksum = cpu_to_le16(chksum); es->modified = true; } @@ -741,59 +736,8 @@ 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_dentry_cached(struct exfat_entry_set_cache *es, + int num) { int off = es->start_off + num * DENTRY_SIZE; struct buffer_head *bh = es->bh[EXFAT_B_TO_BLK(off, es->sb)]; @@ -817,16 +761,14 @@ struct exfat_dentry *exfat_get_dentry_cached( * NULL on failure. */ 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; + int i, num_bh, num_entries, last_name_entry; + unsigned int 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) { @@ -834,42 +776,31 @@ struct exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb, return NULL; } - byte_offset = EXFAT_DEN_TO_B(entry); - ret = exfat_walk_fat_chain(sb, p_dir, byte_offset, &clu); - if (ret) + ep = exfat_get_dentry(sb, p_dir, entry, &bh, &sec); + if (!ep || ep->type != EXFAT_FILE) { + brelse(bh); return NULL; + } - es = kzalloc(sizeof(*es), GFP_KERNEL); - if (!es) + max_entries = max(max_entries, ES_2_ENTRIES); + num_entries = min(ep->dentry.file.num_ext + 1, max_entries); + es = kzalloc(sizeof(*es) + num_entries * sizeof(struct exfat_dentry *), + GFP_KERNEL); + if (!es) { + brelse(bh); return NULL; + } + es->sb = sb; es->modified = false; - - /* 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; - - /* sector offset in cluster */ - sec = EXFAT_B_TO_BLK(byte_offset, sb); - sec += exfat_cluster_to_sector(sbi, clu); - - bh = sb_bread(sb, sec); - if (!bh) - 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)) - goto free_es; - - num_entries = type == ES_ALL_ENTRIES ? - ep->dentry.file.num_ext + 1 : type; es->num_entries = num_entries; + es->de[ENTRY_FILE] = ep; + es->bh[es->num_bh++] = bh; + /* byte offset in sector */ + es->start_off = EXFAT_BLK_OFFSET(EXFAT_DEN_TO_B(entry), sb); - num_bh = EXFAT_B_TO_BLK_ROUND_UP(off + num_entries * DENTRY_SIZE, sb); + clu = exfat_sector_to_cluster(sbi, sec); + num_bh = EXFAT_B_TO_BLK_ROUND_UP(es->start_off + 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)) { @@ -888,14 +819,27 @@ struct exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb, es->bh[es->num_bh++] = bh; } - /* 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)) + ep = exfat_get_dentry_cached(es, ENTRY_STREAM); + if (!ep || ep->type != EXFAT_STREAM) + goto free_es; + es->de[ENTRY_STREAM] = ep; + + last_name_entry = + ENTRY_NAME + ES_STREAM(es).name_len / EXFAT_FILE_NAME_LEN; + for (i = ENTRY_NAME; i < es->num_entries; i++) { + es->de[i] = exfat_get_dentry_cached(es, i); + + if (i <= last_name_entry) { + if (exfat_get_entry_type(es->de[i]) != TYPE_NAME) + goto free_es; + continue; + } + + if (!(exfat_get_entry_type(es->de[i]) & TYPE_SECONDARY)) goto free_es; } - return es; + return es; free_es: exfat_free_dentry_set(es, false); return NULL; @@ -1028,7 +972,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 +1088,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 44dc04520175..0e4cc8ba2f8e 100644 --- a/fs/exfat/exfat_fs.h +++ b/fs/exfat/exfat_fs.h @@ -33,6 +33,12 @@ enum { NLS_NAME_OVERLEN, /* the length is over than its limit */ }; +enum { + ENTRY_FILE, + ENTRY_STREAM, + ENTRY_NAME, +}; + #define EXFAT_HASH_BITS 8 #define EXFAT_HASH_SIZE (1UL << EXFAT_HASH_BITS) @@ -40,7 +46,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 +62,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 +71,8 @@ enum { #define TYPE_BENIGN_SEC 0x0800 #define TYPE_ALL 0x0FFF +#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) @@ -126,6 +134,9 @@ enum { #define BITS_PER_BYTE_MASK 0x7 #define IGNORED_BITS_REMAINED(clu, clu_base) ((1 << ((clu) - (clu_base))) - 1) +#define ES_FILE(es) (es->de[ENTRY_FILE]->dentry.file) +#define ES_STREAM(es) (es->de[ENTRY_STREAM]->dentry.stream) + struct exfat_dentry_namebuf { char *lfn; int lfnbuf_len; /* usually MAX_UNINAME_BUF_SIZE */ @@ -171,7 +182,8 @@ 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_dentry *de[0]; }; struct exfat_dir_entry { @@ -461,7 +473,7 @@ struct exfat_dentry *exfat_get_dentry(struct super_block *sb, struct exfat_dentry *exfat_get_dentry_cached(struct exfat_entry_set_cache *es, int num); 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 4831a39632a1..504ffcaffacc 100644 --- a/fs/exfat/file.c +++ b/fs/exfat/file.c @@ -152,7 +152,6 @@ int __exfat_truncate(struct inode *inode, loff_t new_size) /* update the directory entry */ if (!evict) { struct timespec64 ts; - struct exfat_dentry *ep, *ep2; struct exfat_entry_set_cache *es; int err; @@ -160,32 +159,30 @@ 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); ts = current_time(inode); exfat_set_entry_time(sbi, &ts, - &ep->dentry.file.modify_tz, - &ep->dentry.file.modify_time, - &ep->dentry.file.modify_date, - &ep->dentry.file.modify_time_cs); - ep->dentry.file.attr = cpu_to_le16(ei->attr); + &ES_FILE(es).modify_tz, + &ES_FILE(es).modify_time, + &ES_FILE(es).modify_date, + &ES_FILE(es).modify_time_cs); + ES_FILE(es).attr = cpu_to_le16(ei->attr); /* File size should be zero if there is no cluster allocated */ if (ei->start_clu == EXFAT_EOF_CLUSTER) { - ep2->dentry.stream.valid_size = 0; - ep2->dentry.stream.size = 0; + ES_STREAM(es).valid_size = 0; + ES_STREAM(es).size = 0; } else { - ep2->dentry.stream.valid_size = cpu_to_le64(new_size); - ep2->dentry.stream.size = ep2->dentry.stream.valid_size; + ES_STREAM(es).valid_size = cpu_to_le64(new_size); + ES_STREAM(es).size = ES_STREAM(es).valid_size; } if (new_size == 0) { /* Any directory can not be truncated to zero */ WARN_ON(ei->type != TYPE_FILE); - ep2->dentry.stream.flags = ALLOC_FAT_CHAIN; - ep2->dentry.stream.start_clu = EXFAT_FREE_CLUSTER; + ES_STREAM(es).flags = ALLOC_FAT_CHAIN; + ES_STREAM(es).start_clu = EXFAT_FREE_CLUSTER; } exfat_update_dir_chksum_with_entry_set(es); diff --git a/fs/exfat/inode.c b/fs/exfat/inode.c index 7f90204adef5..a6be7ab76a72 100644 --- a/fs/exfat/inode.c +++ b/fs/exfat/inode.c @@ -20,7 +20,6 @@ static int __exfat_write_inode(struct inode *inode, int sync) { unsigned long long on_disk_size; - struct exfat_dentry *ep, *ep2; struct exfat_entry_set_cache *es = NULL; struct super_block *sb = inode->i_sb; struct exfat_sb_info *sbi = EXFAT_SB(sb); @@ -45,26 +44,24 @@ 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->dentry.file.attr = cpu_to_le16(exfat_make_attr(inode)); + ES_FILE(es).attr = cpu_to_le16(exfat_make_attr(inode)); /* set FILE_INFO structure using the acquired struct exfat_dentry */ exfat_set_entry_time(sbi, &ei->i_crtime, - &ep->dentry.file.create_tz, - &ep->dentry.file.create_time, - &ep->dentry.file.create_date, - &ep->dentry.file.create_time_cs); + &ES_FILE(es).create_tz, + &ES_FILE(es).create_time, + &ES_FILE(es).create_date, + &ES_FILE(es).create_time_cs); exfat_set_entry_time(sbi, &inode->i_mtime, - &ep->dentry.file.modify_tz, - &ep->dentry.file.modify_time, - &ep->dentry.file.modify_date, - &ep->dentry.file.modify_time_cs); + &ES_FILE(es).modify_tz, + &ES_FILE(es).modify_time, + &ES_FILE(es).modify_date, + &ES_FILE(es).modify_time_cs); exfat_set_entry_time(sbi, &inode->i_atime, - &ep->dentry.file.access_tz, - &ep->dentry.file.access_time, - &ep->dentry.file.access_date, + &ES_FILE(es).access_tz, + &ES_FILE(es).access_time, + &ES_FILE(es).access_date, NULL); /* File size should be zero if there is no cluster allocated */ @@ -73,8 +70,8 @@ static int __exfat_write_inode(struct inode *inode, int sync) if (ei->start_clu == EXFAT_EOF_CLUSTER) on_disk_size = 0; - ep2->dentry.stream.valid_size = cpu_to_le64(on_disk_size); - ep2->dentry.stream.size = ep2->dentry.stream.valid_size; + ES_STREAM(es).valid_size = cpu_to_le64(on_disk_size); + ES_STREAM(es).size = ES_STREAM(es).valid_size; exfat_update_dir_chksum_with_entry_set(es); return exfat_free_dentry_set(es, sync); @@ -219,7 +216,6 @@ static int exfat_map_cluster(struct inode *inode, unsigned int clu_offset, *clu = new_clu.dir; if (ei->dir.dir != DIR_DELETED && modified) { - struct exfat_dentry *ep; struct exfat_entry_set_cache *es; int err; @@ -227,17 +223,12 @@ static int exfat_map_cluster(struct inode *inode, unsigned int clu_offset, ES_ALL_ENTRIES); if (!es) return -EIO; - /* get stream entry */ - ep = exfat_get_dentry_cached(es, 1); /* update directory entry */ - ep->dentry.stream.flags = ei->flags; - ep->dentry.stream.start_clu = - cpu_to_le32(ei->start_clu); - ep->dentry.stream.valid_size = - cpu_to_le64(i_size_read(inode)); - ep->dentry.stream.size = - ep->dentry.stream.valid_size; + ES_STREAM(es).flags = ei->flags; + ES_STREAM(es).start_clu = cpu_to_le32(ei->start_clu); + ES_STREAM(es).valid_size = cpu_to_le64(i_size_read(inode)); + ES_STREAM(es).size = ES_STREAM(es).valid_size; exfat_update_dir_chksum_with_entry_set(es); err = exfat_free_dentry_set(es, inode_needs_sync(inode)); diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c index 2aff6605fecc..469fe075dc1f 100644 --- a/fs/exfat/namei.c +++ b/fs/exfat/namei.c @@ -658,25 +658,21 @@ static int exfat_find(struct inode *dir, struct qstr *qname, info->num_subdirs = count; } else { - struct exfat_dentry *ep, *ep2; struct exfat_entry_set_cache *es; 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); - info->type = exfat_get_entry_type(ep); - info->attr = le16_to_cpu(ep->dentry.file.attr); - info->size = le64_to_cpu(ep2->dentry.stream.valid_size); + info->type = exfat_get_entry_type(es->de[ENTRY_FILE]); + info->attr = le16_to_cpu(ES_FILE(es).attr); + info->size = le64_to_cpu(ES_STREAM(es).valid_size); if ((info->type == TYPE_FILE) && (info->size == 0)) { info->flags = ALLOC_NO_FAT_CHAIN; info->start_clu = EXFAT_EOF_CLUSTER; } else { - info->flags = ep2->dentry.stream.flags; - info->start_clu = - le32_to_cpu(ep2->dentry.stream.start_clu); + info->flags = ES_STREAM(es).flags; + info->start_clu = le32_to_cpu(ES_STREAM(es).start_clu); } if (ei->start_clu == EXFAT_FREE_CLUSTER) { @@ -688,19 +684,19 @@ static int exfat_find(struct inode *dir, struct qstr *qname, } exfat_get_entry_time(sbi, &info->crtime, - ep->dentry.file.create_tz, - ep->dentry.file.create_time, - ep->dentry.file.create_date, - ep->dentry.file.create_time_cs); + ES_FILE(es).create_tz, + ES_FILE(es).create_time, + ES_FILE(es).create_date, + ES_FILE(es).create_time_cs); exfat_get_entry_time(sbi, &info->mtime, - ep->dentry.file.modify_tz, - ep->dentry.file.modify_time, - ep->dentry.file.modify_date, - ep->dentry.file.modify_time_cs); + ES_FILE(es).modify_tz, + ES_FILE(es).modify_time, + ES_FILE(es).modify_date, + ES_FILE(es).modify_time_cs); exfat_get_entry_time(sbi, &info->atime, - ep->dentry.file.access_tz, - ep->dentry.file.access_time, - ep->dentry.file.access_date, + ES_FILE(es).access_tz, + ES_FILE(es).access_time, + ES_FILE(es).access_date, 0); exfat_free_dentry_set(es, false);
>>> I prefer to assign validated entries to **de and use it using enum value. >>> struct exfat_dentry **de; >> >> I've tried several implementations that add a struct exfat_dentry type.(*de0 & *de1; *de[2]; etc...) >> The problem with the struct exfat_dentry type is that it is too flexible for type. >> This means weak typing. >> Therefore, when using them, >> de[XXX_FILE]->dentry.file.zzz ... >> It is necessary to re-specify the type. (against the DRY principle) Strong typing prevents use with >> wrong type, at compiling. >> >> I think the approach of using de_file/de_stream could be strongly typed. >> I don't think we need excessive flexibility. > > Could you check the following change ? Thank you so much for your suggestion. > If you think it's okay, please add it to your patch series. I tentatively implemented name length and FileSet-checksum based on your patch. (sorry for the late reply to write this email) As a result, I cannot accept the following points. First: It has no range validation. The main purpose of my patch is to implement Sungjong's suggestions. >> In order to prevent illegal accesses to bh and dentries, it would be better >> to check validation for num and bh. Range over is a common mistakes when handling with variable length objects. Therefore, range validation is required at accessing es->de[]. ------------------------- [name-length example] for (l = 0, n = ENTRY_NAME; l < uniname->name_len; n++) { if (n > es->num_entries || exfat_get_entry_type(es->de[n]) != TYPE_NAME) return -EIO; for (i = 0; l < uniname->name_len && i < EXFAT_FILE_NAME_LEN; i++, l++) uniname->name[l] = le16_to_cpu(es->de[n]->dentry.name.unicode_0_14[i]); } ------------------------- [chksum example] *chksum = exfat_calc_chksum16(es->de[ENTRY_FILE], DENTRY_SIZE, 0, CS_DIR_ENTRY); for (i = 0; i < ES_FILE(es)->num_ext; i++) { if (i > es->num_entries || !(exfat_get_entry_type(es->de[i]) & TYPE_SECONDARY)) return -EIO; *chksum = exfat_calc_chksum16(es->de[1 + i], DENTRY_SIZE, chksum, CS_DEFAULT); } ------------------------- We must be careful not only with the range of the target object(name,dir-entries), but also with the range of es->de[]. However, in most cases, it will work well without such as range validation of es->de[]. Even if we forget it, we are hard to notice. Also, direct access to arrays can be a serious problem(continue to run with memory corruption) when an overrange occurs. A check miss on a null pointer will only kernel-panic. <- This is also serious, but still better, I think. Range verification should be enforced for dir-entries access. And, I think exfat_entry_set_cache should be a fixed size structure. It is easy to detect mistakes at compile time and has good memory efficiency at runtime. Second: Range validation and type validation should not be separated. When I started making this patch, I intended to add only range validation. However, after the caller gets the ep, the type validation follows. Get ep, null check of ep (= range verification), type verification is a series of procedures. There would be no reason to keep them independent anymore. Range and type validation is enforced when the caller uses ep. -- Range validation is the most important fix for this patch. Accessing pre-validated file/stream is a side change. What changes are the biggest problems in my V3 patch? ("On the whole" makes me sad) I wrote a comment in your patch code, so please read it. > diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c > index 573659bfbc55..37e0f92f74b3 100644 > --- a/fs/exfat/dir.c > +++ b/fs/exfat/dir.c > @@ -44,14 +44,12 @@ 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); > - > + for (i = ENTRY_NAME; i < es->num_entries; i++) { > /* end of name entry */ > - if (exfat_get_entry_type(ep) != TYPE_EXTEND) > + if (exfat_get_entry_type(es->de[i]) != TYPE_NAME) > break; > > - exfat_extract_uni_name(ep, uniname); > + exfat_extract_uni_name(es->de[i], uniname); > uniname += EXFAT_FILE_NAME_LEN; > } > > @@ -372,7 +370,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 +386,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 +419,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++) { > @@ -591,16 +589,13 @@ void exfat_update_dir_chksum_with_entry_set(struct exfat_entry_set_cache *es) > { > int chksum_type = CS_DIR_ENTRY, i; > unsigned short chksum = 0; > - struct exfat_dentry *ep; > > for (i = 0; i < es->num_entries; i++) { > - ep = exfat_get_dentry_cached(es, i); > - chksum = exfat_calc_chksum16(ep, DENTRY_SIZE, chksum, > + chksum = exfat_calc_chksum16(es->de[i], DENTRY_SIZE, chksum, > chksum_type); > chksum_type = CS_DEFAULT; > } > - ep = exfat_get_dentry_cached(es, 0); > - ep->dentry.file.checksum = cpu_to_le16(chksum); > + ES_FILE(es).checksum = cpu_to_le16(chksum); > es->modified = true; > } > > @@ -741,59 +736,8 @@ 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_dentry_cached(struct exfat_entry_set_cache *es, > + int num) > { > int off = es->start_off + num * DENTRY_SIZE; > struct buffer_head *bh = es->bh[EXFAT_B_TO_BLK(off, es->sb)]; > @@ -817,16 +761,14 @@ struct exfat_dentry *exfat_get_dentry_cached( > * NULL on failure. > */ > 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; > + int i, num_bh, num_entries, last_name_entry; > + unsigned int 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) { > @@ -834,42 +776,31 @@ struct exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb, > return NULL; > } > > - byte_offset = EXFAT_DEN_TO_B(entry); > - ret = exfat_walk_fat_chain(sb, p_dir, byte_offset, &clu); > - if (ret) > + ep = exfat_get_dentry(sb, p_dir, entry, &bh, &sec); > + if (!ep || ep->type != EXFAT_FILE) { > + brelse(bh); > return NULL; > + } > > - es = kzalloc(sizeof(*es), GFP_KERNEL); > - if (!es) > + max_entries = max(max_entries, ES_2_ENTRIES); > + num_entries = min(ep->dentry.file.num_ext + 1, max_entries); > + es = kzalloc(sizeof(*es) + num_entries * sizeof(struct exfat_dentry *), > + GFP_KERNEL); > + if (!es) { > + brelse(bh); > return NULL; > + } > + > es->sb = sb; > es->modified = false; > - > - /* 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; > - > - /* sector offset in cluster */ > - sec = EXFAT_B_TO_BLK(byte_offset, sb); > - sec += exfat_cluster_to_sector(sbi, clu); > - > - bh = sb_bread(sb, sec); > - if (!bh) > - 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)) > - goto free_es; > - > - num_entries = type == ES_ALL_ENTRIES ? > - ep->dentry.file.num_ext + 1 : type; > es->num_entries = num_entries; > + es->de[ENTRY_FILE] = ep; > + es->bh[es->num_bh++] = bh; > + /* byte offset in sector */ > + es->start_off = EXFAT_BLK_OFFSET(EXFAT_DEN_TO_B(entry), sb); > > - num_bh = EXFAT_B_TO_BLK_ROUND_UP(off + num_entries * DENTRY_SIZE, sb); > + clu = exfat_sector_to_cluster(sbi, sec); > + num_bh = EXFAT_B_TO_BLK_ROUND_UP(es->start_off + 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)) { > @@ -888,14 +819,27 @@ struct exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb, > es->bh[es->num_bh++] = bh; > } > > - /* 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)) > + ep = exfat_get_dentry_cached(es, ENTRY_STREAM); > + if (!ep || ep->type != EXFAT_STREAM) > + goto free_es; > + es->de[ENTRY_STREAM] = ep; The value contained in stream-ext dir-entry should not be used before validating the EntrySet checksum. So I would insert EntrySet checksum validation here. In that case, the checksum verification loop would be followed by the TYPE_NAME verification loop, can you acceptable? > + last_name_entry = > + ENTRY_NAME + ES_STREAM(es).name_len / EXFAT_FILE_NAME_LEN; > + for (i = ENTRY_NAME; i < es->num_entries; i++) { > + es->de[i] = exfat_get_dentry_cached(es, i); > + > + if (i <= last_name_entry) { > + if (exfat_get_entry_type(es->de[i]) != TYPE_NAME) > + goto free_es; > + continue; > + } > + > + if (!(exfat_get_entry_type(es->de[i]) & TYPE_SECONDARY)) > goto free_es; > } > - return es; > > + return es; > free_es: > exfat_free_dentry_set(es, false); > return NULL; > @@ -1028,7 +972,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 +1088,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 44dc04520175..0e4cc8ba2f8e 100644 > --- a/fs/exfat/exfat_fs.h > +++ b/fs/exfat/exfat_fs.h > @@ -33,6 +33,12 @@ enum { > NLS_NAME_OVERLEN, /* the length is over than its limit */ > }; > > +enum { > + ENTRY_FILE, > + ENTRY_STREAM, > + ENTRY_NAME, > +}; This is necessary! With this, some magic numbers will be gone. But, I think it's better to use a name that can be recognized as an offset/index in the EntrySet. And, I think it's better to define this in "exfat_raw.h" > + > #define EXFAT_HASH_BITS 8 > #define EXFAT_HASH_SIZE (1UL << EXFAT_HASH_BITS) > > @@ -40,7 +46,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 +62,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 +71,8 @@ enum { > #define TYPE_BENIGN_SEC 0x0800 > #define TYPE_ALL 0x0FFF > > +#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) > @@ -126,6 +134,9 @@ enum { > #define BITS_PER_BYTE_MASK 0x7 > #define IGNORED_BITS_REMAINED(clu, clu_base) ((1 << ((clu) - (clu_base))) - 1) > > +#define ES_FILE(es) (es->de[ENTRY_FILE]->dentry.file) > +#define ES_STREAM(es) (es->de[ENTRY_STREAM]->dentry.stream) I think that hiding with a macro is simple and good. My V3 patch was a bit tricky. > + > struct exfat_dentry_namebuf { > char *lfn; > int lfnbuf_len; /* usually MAX_UNINAME_BUF_SIZE */ > @@ -171,7 +182,8 @@ 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_dentry *de[0]; > }; > > struct exfat_dir_entry { > @@ -461,7 +473,7 @@ struct exfat_dentry *exfat_get_dentry(struct super_block *sb, > struct exfat_dentry *exfat_get_dentry_cached(struct exfat_entry_set_cache *es, > int num); > 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 4831a39632a1..504ffcaffacc 100644 > --- a/fs/exfat/file.c > +++ b/fs/exfat/file.c > @@ -152,7 +152,6 @@ int __exfat_truncate(struct inode *inode, loff_t new_size) > /* update the directory entry */ > if (!evict) { > struct timespec64 ts; > - struct exfat_dentry *ep, *ep2; > struct exfat_entry_set_cache *es; > int err; > > @@ -160,32 +159,30 @@ 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); > > ts = current_time(inode); > exfat_set_entry_time(sbi, &ts, > - &ep->dentry.file.modify_tz, > - &ep->dentry.file.modify_time, > - &ep->dentry.file.modify_date, > - &ep->dentry.file.modify_time_cs); > - ep->dentry.file.attr = cpu_to_le16(ei->attr); > + &ES_FILE(es).modify_tz, > + &ES_FILE(es).modify_time, > + &ES_FILE(es).modify_date, > + &ES_FILE(es).modify_time_cs); > + ES_FILE(es).attr = cpu_to_le16(ei->attr); > > /* File size should be zero if there is no cluster allocated */ > if (ei->start_clu == EXFAT_EOF_CLUSTER) { > - ep2->dentry.stream.valid_size = 0; > - ep2->dentry.stream.size = 0; > + ES_STREAM(es).valid_size = 0; > + ES_STREAM(es).size = 0; > } else { > - ep2->dentry.stream.valid_size = cpu_to_le64(new_size); > - ep2->dentry.stream.size = ep2->dentry.stream.valid_size; > + ES_STREAM(es).valid_size = cpu_to_le64(new_size); > + ES_STREAM(es).size = ES_STREAM(es).valid_size; > } > > if (new_size == 0) { > /* Any directory can not be truncated to zero */ > WARN_ON(ei->type != TYPE_FILE); > > - ep2->dentry.stream.flags = ALLOC_FAT_CHAIN; > - ep2->dentry.stream.start_clu = EXFAT_FREE_CLUSTER; > + ES_STREAM(es).flags = ALLOC_FAT_CHAIN; > + ES_STREAM(es).start_clu = EXFAT_FREE_CLUSTER; > } > > exfat_update_dir_chksum_with_entry_set(es); > diff --git a/fs/exfat/inode.c b/fs/exfat/inode.c > index 7f90204adef5..a6be7ab76a72 100644 > --- a/fs/exfat/inode.c > +++ b/fs/exfat/inode.c > @@ -20,7 +20,6 @@ > static int __exfat_write_inode(struct inode *inode, int sync) > { > unsigned long long on_disk_size; > - struct exfat_dentry *ep, *ep2; > struct exfat_entry_set_cache *es = NULL; > struct super_block *sb = inode->i_sb; > struct exfat_sb_info *sbi = EXFAT_SB(sb); > @@ -45,26 +44,24 @@ 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->dentry.file.attr = cpu_to_le16(exfat_make_attr(inode)); > + ES_FILE(es).attr = cpu_to_le16(exfat_make_attr(inode)); > > /* set FILE_INFO structure using the acquired struct exfat_dentry */ > exfat_set_entry_time(sbi, &ei->i_crtime, > - &ep->dentry.file.create_tz, > - &ep->dentry.file.create_time, > - &ep->dentry.file.create_date, > - &ep->dentry.file.create_time_cs); > + &ES_FILE(es).create_tz, > + &ES_FILE(es).create_time, > + &ES_FILE(es).create_date, > + &ES_FILE(es).create_time_cs); > exfat_set_entry_time(sbi, &inode->i_mtime, > - &ep->dentry.file.modify_tz, > - &ep->dentry.file.modify_time, > - &ep->dentry.file.modify_date, > - &ep->dentry.file.modify_time_cs); > + &ES_FILE(es).modify_tz, > + &ES_FILE(es).modify_time, > + &ES_FILE(es).modify_date, > + &ES_FILE(es).modify_time_cs); > exfat_set_entry_time(sbi, &inode->i_atime, > - &ep->dentry.file.access_tz, > - &ep->dentry.file.access_time, > - &ep->dentry.file.access_date, > + &ES_FILE(es).access_tz, > + &ES_FILE(es).access_time, > + &ES_FILE(es).access_date, > NULL); > > /* File size should be zero if there is no cluster allocated */ > @@ -73,8 +70,8 @@ static int __exfat_write_inode(struct inode *inode, int sync) > if (ei->start_clu == EXFAT_EOF_CLUSTER) > on_disk_size = 0; > > - ep2->dentry.stream.valid_size = cpu_to_le64(on_disk_size); > - ep2->dentry.stream.size = ep2->dentry.stream.valid_size; > + ES_STREAM(es).valid_size = cpu_to_le64(on_disk_size); > + ES_STREAM(es).size = ES_STREAM(es).valid_size; > > exfat_update_dir_chksum_with_entry_set(es); > return exfat_free_dentry_set(es, sync); > @@ -219,7 +216,6 @@ static int exfat_map_cluster(struct inode *inode, unsigned int clu_offset, > *clu = new_clu.dir; > > if (ei->dir.dir != DIR_DELETED && modified) { > - struct exfat_dentry *ep; > struct exfat_entry_set_cache *es; > int err; > > @@ -227,17 +223,12 @@ static int exfat_map_cluster(struct inode *inode, unsigned int clu_offset, > ES_ALL_ENTRIES); > if (!es) > return -EIO; > - /* get stream entry */ > - ep = exfat_get_dentry_cached(es, 1); > > /* update directory entry */ > - ep->dentry.stream.flags = ei->flags; > - ep->dentry.stream.start_clu = > - cpu_to_le32(ei->start_clu); > - ep->dentry.stream.valid_size = > - cpu_to_le64(i_size_read(inode)); > - ep->dentry.stream.size = > - ep->dentry.stream.valid_size; > + ES_STREAM(es).flags = ei->flags; > + ES_STREAM(es).start_clu = cpu_to_le32(ei->start_clu); > + ES_STREAM(es).valid_size = cpu_to_le64(i_size_read(inode)); > + ES_STREAM(es).size = ES_STREAM(es).valid_size; > > exfat_update_dir_chksum_with_entry_set(es); > err = exfat_free_dentry_set(es, inode_needs_sync(inode)); > diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c > index 2aff6605fecc..469fe075dc1f 100644 > --- a/fs/exfat/namei.c > +++ b/fs/exfat/namei.c > @@ -658,25 +658,21 @@ static int exfat_find(struct inode *dir, struct qstr *qname, > > info->num_subdirs = count; > } else { > - struct exfat_dentry *ep, *ep2; > struct exfat_entry_set_cache *es; > > 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); > > - info->type = exfat_get_entry_type(ep); > - info->attr = le16_to_cpu(ep->dentry.file.attr); > - info->size = le64_to_cpu(ep2->dentry.stream.valid_size); > + info->type = exfat_get_entry_type(es->de[ENTRY_FILE]); > + info->attr = le16_to_cpu(ES_FILE(es).attr); > + info->size = le64_to_cpu(ES_STREAM(es).valid_size); > if ((info->type == TYPE_FILE) && (info->size == 0)) { > info->flags = ALLOC_NO_FAT_CHAIN; > info->start_clu = EXFAT_EOF_CLUSTER; > } else { > - info->flags = ep2->dentry.stream.flags; > - info->start_clu = > - le32_to_cpu(ep2->dentry.stream.start_clu); > + info->flags = ES_STREAM(es).flags; > + info->start_clu = le32_to_cpu(ES_STREAM(es).start_clu); > } > > if (ei->start_clu == EXFAT_FREE_CLUSTER) { > @@ -688,19 +684,19 @@ static int exfat_find(struct inode *dir, struct qstr *qname, > } > > exfat_get_entry_time(sbi, &info->crtime, > - ep->dentry.file.create_tz, > - ep->dentry.file.create_time, > - ep->dentry.file.create_date, > - ep->dentry.file.create_time_cs); > + ES_FILE(es).create_tz, > + ES_FILE(es).create_time, > + ES_FILE(es).create_date, > + ES_FILE(es).create_time_cs); > exfat_get_entry_time(sbi, &info->mtime, > - ep->dentry.file.modify_tz, > - ep->dentry.file.modify_time, > - ep->dentry.file.modify_date, > - ep->dentry.file.modify_time_cs); > + ES_FILE(es).modify_tz, > + ES_FILE(es).modify_time, > + ES_FILE(es).modify_date, > + ES_FILE(es).modify_time_cs); > exfat_get_entry_time(sbi, &info->atime, > - ep->dentry.file.access_tz, > - ep->dentry.file.access_time, > - ep->dentry.file.access_date, > + ES_FILE(es).access_tz, > + ES_FILE(es).access_time, > + ES_FILE(es).access_date, > 0); > exfat_free_dentry_set(es, false); > > BR --- Tetsuhiro Kohada <kohada.t2@gmail.com>
> Second: Range validation and type validation should not be separated. > When I started making this patch, I intended to add only range validation. > However, after the caller gets the ep, the type validation follows. > Get ep, null check of ep (= range verification), type verification is a series of procedures. > There would be no reason to keep them independent anymore. > Range and type validation is enforced when the caller uses ep. You can add a validate flags as argument of exfat_get_dentry_set(), e.g. none, basic and strict. none : only range validation. basic : range + type validation. strict : range + type + checksum and name length, etc. > > - /* 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)) > > + ep = exfat_get_dentry_cached(es, ENTRY_STREAM); > > + if (!ep || ep->type != EXFAT_STREAM) > > + goto free_es; > > + es->de[ENTRY_STREAM] = ep; > > The value contained in stream-ext dir-entry should not be used before validating the EntrySet checksum. > So I would insert EntrySet checksum validation here. > In that case, the checksum verification loop would be followed by the TYPE_NAME verification loop, can > you acceptable? Yes. That would be great. Thanks! > > > > diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h index > > 44dc04520175..0e4cc8ba2f8e 100644 > > --- a/fs/exfat/exfat_fs.h > > +++ b/fs/exfat/exfat_fs.h > > @@ -33,6 +33,12 @@ enum { > > NLS_NAME_OVERLEN, /* the length is over than its limit */ > > }; > > > > +enum { > > + ENTRY_FILE, > > + ENTRY_STREAM, > > + ENTRY_NAME, > > +}; > > This is necessary! > With this, some magic numbers will be gone. > But, I think it's better to use a name that can be recognized as an offset/index in the EntrySet. > And, I think it's better to define this in "exfat_raw.h" Okay, You can rename it and move it to there.
On 2020/08/26 10:03, Namjae Jeon wrote: >> Second: Range validation and type validation should not be separated. >> When I started making this patch, I intended to add only range validation. >> However, after the caller gets the ep, the type validation follows. >> Get ep, null check of ep (= range verification), type verification is a series of procedures. >> There would be no reason to keep them independent anymore. >> Range and type validation is enforced when the caller uses ep. > You can add a validate flags as argument of exfat_get_dentry_set(), e.g. none, basic and strict. > none : only range validation. > basic : range + type validation. > strict : range + type + checksum and name length, etc. Currently, various types of verification will not be needed. Let's add it when we need it. > >>> - /* 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)) >>> + ep = exfat_get_dentry_cached(es, ENTRY_STREAM); >>> + if (!ep || ep->type != EXFAT_STREAM) >>> + goto free_es; >>> + es->de[ENTRY_STREAM] = ep; >> >> The value contained in stream-ext dir-entry should not be used before validating the EntrySet checksum. >> So I would insert EntrySet checksum validation here. >> In that case, the checksum verification loop would be followed by the TYPE_NAME verification loop, can >> you acceptable? > Yes. That would be great. OK. I'll add TYPE_NAME verification after checksum verification, in next patch. However, I think it is enough to validate TYPE_NAME when extracting name. Could you please tell me why you think you need TYPE_NAME validation here? BR --- Tetsuhiro Kohada <kohada.t2@gmail.com> >
> On 2020/08/26 10:03, Namjae Jeon wrote: > >> Second: Range validation and type validation should not be separated. > >> When I started making this patch, I intended to add only range validation. > >> However, after the caller gets the ep, the type validation follows. > >> Get ep, null check of ep (= range verification), type verification is a series of procedures. > >> There would be no reason to keep them independent anymore. > >> Range and type validation is enforced when the caller uses ep. > > You can add a validate flags as argument of exfat_get_dentry_set(), e.g. none, basic and strict. > > none : only range validation. > > basic : range + type validation. > > strict : range + type + checksum and name length, etc. > > Currently, various types of verification will not be needed. > Let's add it when we need it. > > > >>> - /* 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)) > >>> + ep = exfat_get_dentry_cached(es, ENTRY_STREAM); > >>> + if (!ep || ep->type != EXFAT_STREAM) > >>> + goto free_es; > >>> + es->de[ENTRY_STREAM] = ep; > >> > >> The value contained in stream-ext dir-entry should not be used before validating the EntrySet > checksum. > >> So I would insert EntrySet checksum validation here. > >> In that case, the checksum verification loop would be followed by the > >> TYPE_NAME verification loop, can you acceptable? > > Yes. That would be great. > > OK. > I'll add TYPE_NAME verification after checksum verification, in next patch. > However, I think it is enough to validate TYPE_NAME when extracting name. > Could you please tell me why you think you need TYPE_NAME validation here? I've told you on previous mail. This function should return validated dentry set after checking file->stream->name in sequence. > > > BR > --- > Tetsuhiro Kohada <kohada.t2@gmail.com> > >
Thank you for quick reply! On 2020/08/26 13:19, Namjae Jeon wrote: >> On 2020/08/26 10:03, Namjae Jeon wrote: >>>> Second: Range validation and type validation should not be separated. >>>> When I started making this patch, I intended to add only range validation. >>>> However, after the caller gets the ep, the type validation follows. >>>> Get ep, null check of ep (= range verification), type verification is a series of procedures. >>>> There would be no reason to keep them independent anymore. >>>> Range and type validation is enforced when the caller uses ep. >>> You can add a validate flags as argument of exfat_get_dentry_set(), e.g. none, basic and strict. >>> none : only range validation. >>> basic : range + type validation. >>> strict : range + type + checksum and name length, etc. >> >> Currently, various types of verification will not be needed. >> Let's add it when we need it. >>> >>>>> - /* 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)) >>>>> + ep = exfat_get_dentry_cached(es, ENTRY_STREAM); >>>>> + if (!ep || ep->type != EXFAT_STREAM) >>>>> + goto free_es; >>>>> + es->de[ENTRY_STREAM] = ep; >>>> >>>> The value contained in stream-ext dir-entry should not be used before validating the EntrySet >> checksum. >>>> So I would insert EntrySet checksum validation here. >>>> In that case, the checksum verification loop would be followed by the >>>> TYPE_NAME verification loop, can you acceptable? >>> Yes. That would be great. >> >> OK. >> I'll add TYPE_NAME verification after checksum verification, in next patch. >> However, I think it is enough to validate TYPE_NAME when extracting name. >> Could you please tell me why you think you need TYPE_NAME validation here? > I've told you on previous mail. This function should return validated dentry set after checking > file->stream->name in sequence. Yes. I understand that the current implementation checks in that order. Sorry, my question was unclear. Why do you think you should leave the TYPE_NAME validation in this function? What kind of problem are you worried about if this function does not validate TYPE_NAME? (for preserve the current behavior?) Don't worry, I will add TYPE_NAME verification to the v4 patch. I will post it later today. BR --- Tetsuhiro Kohada <kohada.t2@gmail.com>
> Thank you for quick reply! > > On 2020/08/26 13:19, Namjae Jeon wrote: > >> On 2020/08/26 10:03, Namjae Jeon wrote: > >>>> Second: Range validation and type validation should not be separated. > >>>> When I started making this patch, I intended to add only range validation. > >>>> However, after the caller gets the ep, the type validation follows. > >>>> Get ep, null check of ep (= range verification), type verification is a series of procedures. > >>>> There would be no reason to keep them independent anymore. > >>>> Range and type validation is enforced when the caller uses ep. > >>> You can add a validate flags as argument of exfat_get_dentry_set(), e.g. none, basic and strict. > >>> none : only range validation. > >>> basic : range + type validation. > >>> strict : range + type + checksum and name length, etc. > >> > >> Currently, various types of verification will not be needed. > >> Let's add it when we need it. > >>> > >>>>> - /* 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)) > >>>>> + ep = exfat_get_dentry_cached(es, ENTRY_STREAM); > >>>>> + if (!ep || ep->type != EXFAT_STREAM) > >>>>> + goto free_es; > >>>>> + es->de[ENTRY_STREAM] = ep; > >>>> > >>>> The value contained in stream-ext dir-entry should not be used > >>>> before validating the EntrySet > >> checksum. > >>>> So I would insert EntrySet checksum validation here. > >>>> In that case, the checksum verification loop would be followed by > >>>> the TYPE_NAME verification loop, can you acceptable? > >>> Yes. That would be great. > >> > >> OK. > >> I'll add TYPE_NAME verification after checksum verification, in next patch. > >> However, I think it is enough to validate TYPE_NAME when extracting name. > >> Could you please tell me why you think you need TYPE_NAME validation here? > > I've told you on previous mail. This function should return validated > > dentry set after checking > > file->stream->name in sequence. > > Yes. I understand that the current implementation checks in that order. > Sorry, my question was unclear. > Why do you think you should leave the TYPE_NAME validation in this function? > What kind of problem are you worried about if this function does not validate TYPE_NAME? > (for preserve the current behavior?) We have not checked the problem when it is removed because it was implemented according to the specification from the beginning. And your v3 patch are already checking the name entries as TYPE_SECONDARY. And it check them with TYPE_NAME again in exfat_get_uniname_from_ext_entry(). If you check TYPE_NAME with stream->name_len, We don't need to perform the loop for extracting filename from the name entries if stream->name_len or name entry is invalid. And I request to prove why we do not need to validate name entries in this function calling from somewhere. So as I suggested earlier, You can make it with an argument flags so that we skip the validation. > > Don't worry, I will add TYPE_NAME verification to the v4 patch. > I will post it later today. Sound good. > > BR > --- > Tetsuhiro Kohada <kohada.t2@gmail.com>
>>>>>>> - /* 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)) >>>>>>> + ep = exfat_get_dentry_cached(es, ENTRY_STREAM); >>>>>>> + if (!ep || ep->type != EXFAT_STREAM) >>>>>>> + goto free_es; >>>>>>> + es->de[ENTRY_STREAM] = ep; >>>>>> >>>>>> The value contained in stream-ext dir-entry should not be used >>>>>> before validating the EntrySet >>>> checksum. >>>>>> So I would insert EntrySet checksum validation here. >>>>>> In that case, the checksum verification loop would be followed by >>>>>> the TYPE_NAME verification loop, can you acceptable? >>>>> Yes. That would be great. >>>> >>>> OK. >>>> I'll add TYPE_NAME verification after checksum verification, in next patch. >>>> However, I think it is enough to validate TYPE_NAME when extracting name. >>>> Could you please tell me why you think you need TYPE_NAME validation here? >>> I've told you on previous mail. This function should return validated >>> dentry set after checking >>> file->stream->name in sequence. >> >> Yes. I understand that the current implementation checks in that order. >> Sorry, my question was unclear. >> Why do you think you should leave the TYPE_NAME validation in this function? >> What kind of problem are you worried about if this function does not validate TYPE_NAME? >> (for preserve the current behavior?) > We have not checked the problem when it is removed because it was implemented > according to the specification from the beginning. I understand that the main reason to validate TYPE_NAME here is "according to the specification". (No one knows the actual problem) First, we should validate as 'dir-entry set' by SecondaryCount and SetChecksum described in "6.3 Generic Primary DirectoryEntry Template". Next, description about validity of 'File dir-entry set' is ... 7.4 File Directory Entry: ... For a File directory entry to be valid, exactly one Stream Extension directory entry and at least one File Name directory entry must immediately follow the File directory entry. 7.7 File Name Directory Entry: ... File Name directory entries are valid only if they immediately follow the Stream Extension directory entry as a consecutive series. It is possible to validate the above correctly, with either exfat_get_dentry_set() or exfat_get_uniname_from_name_entries(). Is this wrong? > And your v3 patch are > already checking the name entries as TYPE_SECONDARY. And it check them with > TYPE_NAME again in exfat_get_uniname_from_ext_entry(). This is according to "6.3 Generic Primary DirectoryEntry Template". "6.3 Generic Primary DirectoryEntry Template" only required TYPE_SECONDARY. In v3, there is no checksum validation yet. > If you check TYPE_NAME > with stream->name_len, We don't need to perform the loop for extracting > filename from the name entries if stream->name_len or name entry is invalid. Don't worry, it's a rare case. (Do you care about the run-time costs?) > And I request to prove why we do not need to validate name entries in this > function calling from somewhere. If you need, it's okey to validate in both. However, name-length and type validation and name-extraction should not be separated. These are closely related, so these should be placed physically and temporally close. Well, why it's unnecessary. Both can be validate correctly, as I wrote before. And, I don't really trust the verification with TYPE_NAME. (reliability of validation as 'file dir-entry set' by checksum is much higher) > So as I suggested earlier, You can make it > with an argument flags so that we skip the validation. No need skip the validation, I think. The run-time costs for validation are pretty low. The reason I want to remove the validation is because I want to keep the code simple. (KISS principle) BR --- Tetsuhiro Kohada <kohada.t2@gmail.com>
diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index 573659bfbc55..91cdbede0fd1 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,13 +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->dentry.file.checksum = cpu_to_le16(chksum); + es->de_file->checksum = cpu_to_le16(chksum); es->modified = true; } @@ -741,92 +737,64 @@ 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. */ 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 +812,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 +829,13 @@ 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->de_file = &ep->dentry.file; + es->num_entries = min(es->de_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 +855,16 @@ 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)) + ep = exfat_get_validated_dentry(es, 1, TYPE_STREAM); + if (!ep) + goto free_es; + es->de_stream = &ep->dentry.stream; + + 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 +999,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 +1115,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 95d717f8620c..b88b7abc25bd 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,9 @@ 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_de_file *de_file; + struct exfat_de_stream *de_stream; }; struct exfat_dir_entry { @@ -458,10 +463,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/exfat_raw.h b/fs/exfat/exfat_raw.h index 6aec6288e1f2..b2ece0bf4ecd 100644 --- a/fs/exfat/exfat_raw.h +++ b/fs/exfat/exfat_raw.h @@ -105,7 +105,7 @@ struct boot_sector { struct exfat_dentry { __u8 type; union { - struct { + struct exfat_de_file { __u8 num_ext; __le16 checksum; __le16 attr; @@ -123,7 +123,7 @@ struct exfat_dentry { __u8 access_tz; __u8 reserved2[7]; } __packed file; /* file directory entry */ - struct { + struct exfat_de_stream { __u8 flags; __u8 reserved1; __u8 name_len; @@ -134,17 +134,17 @@ struct exfat_dentry { __le32 start_clu; __le64 size; } __packed stream; /* stream extension directory entry */ - struct { + struct exfat_de_name { __u8 flags; __le16 unicode_0_14[EXFAT_FILE_NAME_LEN]; } __packed name; /* file name directory entry */ - struct { + struct exfat_de_bitmap { __u8 flags; __u8 reserved[18]; __le32 start_clu; __le64 size; } __packed bitmap; /* allocation bitmap directory entry */ - struct { + struct exfat_de_upcase { __u8 reserved1[3]; __le32 checksum; __u8 reserved2[12]; diff --git a/fs/exfat/file.c b/fs/exfat/file.c index f41f523a58ad..04f6cc79ed43 100644 --- a/fs/exfat/file.c +++ b/fs/exfat/file.c @@ -152,7 +152,6 @@ int __exfat_truncate(struct inode *inode, loff_t new_size) /* update the directory entry */ if (!evict) { struct timespec64 ts; - struct exfat_dentry *ep, *ep2; struct exfat_entry_set_cache *es; int err; @@ -160,32 +159,30 @@ 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); ts = current_time(inode); exfat_set_entry_time(sbi, &ts, - &ep->dentry.file.modify_tz, - &ep->dentry.file.modify_time, - &ep->dentry.file.modify_date, - &ep->dentry.file.modify_time_cs); - ep->dentry.file.attr = cpu_to_le16(ei->attr); + &es->de_file->modify_tz, + &es->de_file->modify_time, + &es->de_file->modify_date, + &es->de_file->modify_time_cs); + es->de_file->attr = cpu_to_le16(ei->attr); /* File size should be zero if there is no cluster allocated */ if (ei->start_clu == EXFAT_EOF_CLUSTER) { - ep2->dentry.stream.valid_size = 0; - ep2->dentry.stream.size = 0; + es->de_stream->valid_size = 0; + es->de_stream->size = 0; } else { - ep2->dentry.stream.valid_size = cpu_to_le64(new_size); - ep2->dentry.stream.size = ep2->dentry.stream.valid_size; + es->de_stream->valid_size = cpu_to_le64(new_size); + es->de_stream->size = es->de_stream->valid_size; } if (new_size == 0) { /* Any directory can not be truncated to zero */ WARN_ON(ei->type != TYPE_FILE); - ep2->dentry.stream.flags = ALLOC_FAT_CHAIN; - ep2->dentry.stream.start_clu = EXFAT_FREE_CLUSTER; + es->de_stream->flags = ALLOC_FAT_CHAIN; + es->de_stream->start_clu = EXFAT_FREE_CLUSTER; } exfat_update_dir_chksum_with_entry_set(es); diff --git a/fs/exfat/inode.c b/fs/exfat/inode.c index 7f90204adef5..358457c82270 100644 --- a/fs/exfat/inode.c +++ b/fs/exfat/inode.c @@ -20,7 +20,6 @@ static int __exfat_write_inode(struct inode *inode, int sync) { unsigned long long on_disk_size; - struct exfat_dentry *ep, *ep2; struct exfat_entry_set_cache *es = NULL; struct super_block *sb = inode->i_sb; struct exfat_sb_info *sbi = EXFAT_SB(sb); @@ -45,26 +44,24 @@ 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->dentry.file.attr = cpu_to_le16(exfat_make_attr(inode)); + es->de_file->attr = cpu_to_le16(exfat_make_attr(inode)); /* set FILE_INFO structure using the acquired struct exfat_dentry */ exfat_set_entry_time(sbi, &ei->i_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_set_entry_time(sbi, &inode->i_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_set_entry_time(sbi, &inode->i_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, NULL); /* File size should be zero if there is no cluster allocated */ @@ -73,8 +70,8 @@ static int __exfat_write_inode(struct inode *inode, int sync) if (ei->start_clu == EXFAT_EOF_CLUSTER) on_disk_size = 0; - ep2->dentry.stream.valid_size = cpu_to_le64(on_disk_size); - ep2->dentry.stream.size = ep2->dentry.stream.valid_size; + es->de_stream->valid_size = cpu_to_le64(on_disk_size); + es->de_stream->size = es->de_stream->valid_size; exfat_update_dir_chksum_with_entry_set(es); return exfat_free_dentry_set(es, sync); @@ -219,7 +216,6 @@ static int exfat_map_cluster(struct inode *inode, unsigned int clu_offset, *clu = new_clu.dir; if (ei->dir.dir != DIR_DELETED && modified) { - struct exfat_dentry *ep; struct exfat_entry_set_cache *es; int err; @@ -227,17 +223,12 @@ static int exfat_map_cluster(struct inode *inode, unsigned int clu_offset, ES_ALL_ENTRIES); if (!es) return -EIO; - /* get stream entry */ - ep = exfat_get_dentry_cached(es, 1); - - /* update directory entry */ - ep->dentry.stream.flags = ei->flags; - ep->dentry.stream.start_clu = - cpu_to_le32(ei->start_clu); - ep->dentry.stream.valid_size = - cpu_to_le64(i_size_read(inode)); - ep->dentry.stream.size = - ep->dentry.stream.valid_size; + + /* update stream directory entry */ + es->de_stream->flags = ei->flags; + es->de_stream->start_clu = cpu_to_le32(ei->start_clu); + es->de_stream->valid_size = cpu_to_le64(i_size_read(inode)); + es->de_stream->size = es->de_stream->valid_size; exfat_update_dir_chksum_with_entry_set(es); err = exfat_free_dentry_set(es, inode_needs_sync(inode)); diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c index e73f20f66cb2..a65d60ef93f4 100644 --- a/fs/exfat/namei.c +++ b/fs/exfat/namei.c @@ -658,25 +658,21 @@ static int exfat_find(struct inode *dir, struct qstr *qname, info->num_subdirs = count; } else { - struct exfat_dentry *ep, *ep2; struct exfat_entry_set_cache *es; 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); - info->type = exfat_get_entry_type(ep); - info->attr = le16_to_cpu(ep->dentry.file.attr); - info->size = le64_to_cpu(ep2->dentry.stream.valid_size); + info->attr = le16_to_cpu(es->de_file->attr); + info->type = (info->attr & ATTR_SUBDIR) ? TYPE_DIR : TYPE_FILE; + info->size = le64_to_cpu(es->de_stream->valid_size); if ((info->type == TYPE_FILE) && (info->size == 0)) { info->flags = ALLOC_NO_FAT_CHAIN; info->start_clu = EXFAT_EOF_CLUSTER; } else { - info->flags = ep2->dentry.stream.flags; - info->start_clu = - le32_to_cpu(ep2->dentry.stream.start_clu); + info->flags = es->de_stream->flags; + info->start_clu = le32_to_cpu(es->de_stream->start_clu); } if (ei->start_clu == EXFAT_FREE_CLUSTER) { @@ -688,19 +684,19 @@ static int exfat_find(struct inode *dir, struct qstr *qname, } exfat_get_entry_time(sbi, &info->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, &info->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, &info->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); exfat_free_dentry_set(es, false);
Add validation for num, bh and type on getting dir-entry. 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. Pre-Validated 'file' and 'stream-ext' dir-entries are provided as member variables of exfat_entry_set_cache. And, rename TYPE_EXTEND to TYPE_NAME. Suggested-by: Sungjong Seo <sj1557.seo@samsung.com> Suggested-by: Namjae Jeon <namjae.jeon@samsung.com> Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com> --- Changes in v2 - Change verification order - Verification loop start with index 2 Changes in v3 - Fix indent - Fix comment of exfat_get_dentry_set() - Add de_file/de_stream in exfat_entry_set_cache - Add srtuct tag name for each dir-entry type in exfat_dentry - Add description about de_file/de_stream to commit-log fs/exfat/dir.c | 147 +++++++++++++++++-------------------------- fs/exfat/exfat_fs.h | 17 +++-- fs/exfat/exfat_raw.h | 10 +-- fs/exfat/file.c | 25 ++++---- fs/exfat/inode.c | 49 ++++++--------- fs/exfat/namei.c | 36 +++++------ 6 files changed, 122 insertions(+), 162 deletions(-)