Re: [PATCH v3] exfat: integrates dir-entry getting and validation
- /* 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
RE: [PATCH v3] exfat: integrates dir-entry getting and validation
> 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
Re: [PATCH v3] exfat: integrates dir-entry getting and validation
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
RE: [PATCH v3] exfat: integrates dir-entry getting and validation
> 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
> >
Re: [PATCH v3] exfat: integrates dir-entry getting and validation
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
RE: [PATCH v3] exfat: integrates dir-entry getting and validation
> 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.
Re: [PATCH v3] exfat: integrates dir-entry getting and validation
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_
RE: [PATCH v3] exfat: integrates dir-entry getting and validation
> 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;
-
Re: [PATCH v3] exfat: integrates dir-entry getting and validation
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
RE: [PATCH v3] exfat: integrates dir-entry getting and validation
>
> +#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;
> };
RE: [PATCH v3] 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 as member > variables of exfat_entry_set_cache. > > And, rename TYPE_EXTEND to TYPE_NAME. > > Suggested-by: Sungjong Seo > Suggested-by: Namjae Jeon > Signed-off-by: Tetsuhiro Kohada Reviewed-by: Sungjong Seo 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(-)
[PATCH v3] 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 as member
variables of exfat_entry_set_cache.
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
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(-)
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;
-
