Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Tetsuhiro Kohada <kohada.t2@gmail.com>
To: Namjae Jeon <namjae.jeon@samsung.com>
Cc: kohada.tetsuhiro@dc.mitsubishielectric.co.jp,
	mori.takahiro@ab.mitsubishielectric.co.jp,
	motai.hirotaka@aj.mitsubishielectric.co.jp,
	'Sungjong Seo' <sj1557.seo@samsung.com>,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] exfat: integrates dir-entry getting and validation
Date: Thu, 27 Aug 2020 18:59:48 +0900	[thread overview]
Message-ID: <2b4c4409-7cb8-1651-3966-569636bcc429@gmail.com> (raw)
In-Reply-To: <004301d67b7a$ff0dcf50$fd296df0$@samsung.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>


      reply	other threads:[~2020-08-27 10:00 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CGME20200806010249epcas1p18fd6e3febad305975b43e1b55b56bcae@epcas1p1.samsung.com>
2020-08-06  1:02 ` Tetsuhiro Kohada
2020-08-08 16:35   ` Sungjong Seo
2020-08-10  6:10   ` Namjae Jeon
2020-08-12 13:25     ` Tetsuhiro Kohada
2020-08-21  6:53       ` Namjae Jeon
2020-08-25  8:21         ` Tetsuhiro Kohada
2020-08-26  1:03           ` Namjae Jeon
2020-08-26  2:56             ` Tetsuhiro Kohada
2020-08-26  4:19               ` Namjae Jeon
2020-08-26  6:07                 ` Tetsuhiro Kohada
2020-08-26  7:32                   ` Namjae Jeon
2020-08-27  9:59                     ` Tetsuhiro Kohada [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2b4c4409-7cb8-1651-3966-569636bcc429@gmail.com \
    --to=kohada.t2@gmail.com \
    --cc=kohada.tetsuhiro@dc.mitsubishielectric.co.jp \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mori.takahiro@ab.mitsubishielectric.co.jp \
    --cc=motai.hirotaka@aj.mitsubishielectric.co.jp \
    --cc=namjae.jeon@samsung.com \
    --cc=sj1557.seo@samsung.com \
    --subject='Re: [PATCH v3] exfat: integrates dir-entry getting and validation' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).