Re: [PATCH v4 1/5] exfat: integrates dir-entry getting and validation
Thank you for your quick review.
On 2020/08/27 12:19, Namjae Jeon wrote:
+ i = ES_INDEX_NAME;
+ while ((ep = exfat_get_validated_dentry(es, i++, TYPE_NAME))) {
Please find the way to access name entries like ep_file, ep_stream
without calling exfat_get_validated_dentry().
Hmm, it's a hard order.
I can't separate length/type validation and extraction.
Sorry, I have no good idea.
@@ -590,17 +587,16 @@ int exfat_remove_entries(struct inode *inode, struct
exfat_chain *p_dir, 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;
+ u16 chksum = 0;
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);
Ditto, You do not need to repeatedly call exfat_get_validated_dentry() for the
entries
which got from exfat_get_dentry_set().
Even if I could do that, it would be very difficult to implement a checksum
patch.
It is also difficult to use for rename, move, delete.
(these also have no verification of neme-length and set-checksum)
/* 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), ))
- goto free_es;
+ es->ep_stream = exfat_get_validated_dentry(es, ES_INDEX_STREAM,
TYPE_STREAM);
+ if (!es->ep_stream)
+ goto free_es;
+
+ if (max_entries == ES_ALL_ENTRIES) {
+ for (i = 0; i < ES_FILE(es).num_ext; i++)
+ if (!exfat_get_validated_dentry(es, ES_INDEX_STREAM +
i, TYPE_SECONDARY))
+ goto free_es;
+ for (i = 0; i * EXFAT_FILE_NAME_LEN < ES_STREAM(es).name_len;
i++)
+ if (!exfat_get_validated_dentry(es, ES_INDEX_NAME + i,
TYPE_NAME))
+ goto free_es;
Why do you unnecessarily check entries with two loops?
Please refer to the patch I sent.
This order is possible.
However, TYPE_SECONDARY loop will be back as checksum loop.
In the next patch, I can fix the 'TYPE_SECONDARY loop' order.
do you need it?
BR
---
Tetsuhiro Kohada
RE: [PATCH v4 1/5] exfat: integrates dir-entry getting and validation
> + i = ES_INDEX_NAME;
> + while ((ep = exfat_get_validated_dentry(es, i++, TYPE_NAME))) {
Please find the way to access name entries like ep_file, ep_stream
without calling exfat_get_validated_dentry().
> 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++) { @@ -550,7 +547,7 @@ int
> exfat_init_ext_entry(struct
> inode *inode, struct exfat_chain *p_dir,
> exfat_update_bh(bh, sync);
> brelse(bh);
>
> - for (i = EXFAT_FIRST_CLUSTER; i < num_entries; i++) {
> + for (i = ES_INDEX_NAME; i < num_entries; i++) {
> ep = exfat_get_dentry(sb, p_dir, entry + i, , );
> if (!ep)
> return -EIO;
> @@ -590,17 +587,16 @@ int exfat_remove_entries(struct inode *inode, struct
> exfat_chain *p_dir, 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;
> + u16 chksum = 0;
> 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);
Ditto, You do not need to repeatedly call exfat_get_validated_dentry() for the
entries
which got from exfat_get_dentry_set().
> 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_FILE(es).checksum = cpu_to_le16(chksum);
> es->modified = true;
> }
>
> 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 +811,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 +828,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), ))
> + es->ep_file = exfat_get_validated_dentry(es, ES_INDEX_FILE, TYPE_FILE);
> + if (!es->ep_file)
> goto free_es;
> + es->num_entries = min(ES_FILE(es).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
[PATCH v4 1/5] exfat: integrates dir-entry getting and validation
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 via
ES_FILE/ES_STREAM macros.
And, rename TYPE_EXTEND to TYPE_NAME.
Suggested-by: Sungjong Seo
Suggested-by: Namjae Jeon
Signed-off-by: Tetsuhiro Kohada
---
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
Changes in v4
- Replace de_file/de_stream with ep_file/ep_stream in exfat_entry_set_cache
- Remove srtuct tag names added in v3
- Add ES_INDEX_XXX macro definitions
- Add ES_FILE/ES_STREAM macro definitions
- Replace some EXFAT_FIRST_CLUSTER with ES_INDEX_NAME
- Modify commit-log
fs/exfat/dir.c | 155 ++-
fs/exfat/exfat_fs.h | 19 --
fs/exfat/exfat_raw.h | 14 ++--
fs/exfat/file.c | 25 +++
fs/exfat/inode.c | 49 ++
fs/exfat/namei.c | 36 +-
6 files changed, 132 insertions(+), 166 deletions(-)
diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c
index 573659bfbc55..bb3c20bac422 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 = ES_INDEX_NAME;
+ 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++) {
@@ -550,7 +547,7 @@ int exfat_init_ext_entry(struct inode *inode, struct
exfat_chain *p_dir,
exfat_update_bh(bh, sync);
brelse(bh);
- for (i = EXFAT_FIRST_CLUSTER; i < num_entries; i++) {
+ for (i = ES_INDEX_NAME; i < num_entries; i++) {
ep = exfat_get_dentry(sb, p_dir, entry + i, , );
if (!ep)
return -EIO;
@@ -590,17 +587,16 @@ int exfat_remove_entries(struct inode *inode, struct
exfat_chain *p_dir,
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;
+ u16 chksum = 0;
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_FILE(es).checksum = cpu_to_le16(chksum);
es->modified = true;
}
@@ -741,92 +737,63 @@ struct exfat_dentry *exfat_get_dentry(struct super_block
*sb,
