Message-ID: <OSBPR01MB4535529FC60111A075E3FC1F905F0@OSBPR01MB4535.jpnprd01.prod.outlook.com>
Date:   Mon, 17 Aug 2020 09:26:43 +0000
From:   "Kohada.Tetsuhiro@...MitsubishiElectric.co.jp" 
        <Kohada.Tetsuhiro@...MitsubishiElectric.co.jp>
To:     'Namjae Jeon' <namjae.jeon@...sung.com>
CC:     "Mori.Takahiro@...MitsubishiElectric.co.jp" 
        <Mori.Takahiro@...MitsubishiElectric.co.jp>,
        "Motai.Hirotaka@...MitsubishiElectric.co.jp" 
        <Motai.Hirotaka@...MitsubishiElectric.co.jp>,
        'Sungjong Seo' <sj1557.seo@...sung.com>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 1/2] exfat: add NameLength check when extracting name

Thank you for your reply.

> > >> -static void exfat_get_uniname_from_ext_entry(struct super_block *sb,
> > >> -		struct exfat_chain *p_dir, int entry, unsigned short *uniname)
> > >> +static int exfat_get_uniname_from_name_entries(struct exfat_entry_set_cache *es,
> > >> +		struct exfat_uni_name *uniname)
> > >>   {
> > >> -	int i;
> > >> -	struct exfat_entry_set_cache *es;
> > >> +	int n, l, i;
> > >>   	struct exfat_dentry *ep;
> > >>
> > >> -	es = exfat_get_dentry_set(sb, p_dir, entry, ES_ALL_ENTRIES);
> > >> -	if (!es)
> > >> -		return;
> > >> +	uniname->name_len = es->de_stream->name_len;
> > >> +	if (uniname->name_len == 0)
> > >> +		return -EIO;
> > > Can we validate ->name_len and name entry ->type in exfat_get_dentry_set() ?
> >
> > Yes.
> > As I wrote in a previous email, entry type validation, name-length
> > validation, and name extraction should not be separated, so implement all of these in exfat_get_dentry_set().
> > It can be easily implemented by adding uniname to
> > exfat_entry_set_cache and calling
> > exfat_get_uniname_from_name_entries() from exfat_get_dentry_set().
> No, We can check stream->name_len and name entry type in exfat_get_dentry_set().

I have no objection to that point.

> And you are already checking entry type with TYPE_SECONDARY in exfat_get_dentry_set(). Why do we have to check twice?

This verification is according to the description in '6.3 Generic Primary DirectoryEntry Template'.
The EntrySet is composed of one primary dir-entry and multiple Secondary dir-entries. 
We can validate the EntrySet by SecondaryCount and SetChecksum recorded in the Primary dir-entry.

The EntrySet checksum validation and dir-entries order validation are according to different descriptions. 
Therefore, it is not a double check.


> > However, that would be over-implementation.
> > Not all callers of exfat_get_dentry_set() need a name.
> Where? It will not checked with ES_2_ENTRIES.

The following functions don't require name.
__exfat_truncate()
__exfat_write_inode()
exfat_map_cluster()
exfat_find()


> > It is enough to validate the name when it is needed.
> > This is a file-system driver, not fsck.
> Sorry, I don't understand what you are talking about. If there is a problem in ondisk-metadata, Filesystem should return
> error.

My explanation may have been inappropriate.
(Verifier is a better metaphor than fsck)
Essentially, the purpose of file-system driver is not to detect inconsistencies.
Of course, FSD should return error when it detects an inconsistency, as you say.
However, I think it is no-need for active inconsistency detection.


> > Validation is possible in exfat_get_dentry_set(), but unnecessary.
> >
> > Why do you want to validate the name in exfat_get_dentry_set()?
> exfat_get_dentry_set validates file, stream entry. 

> And you are trying to check name entries with type_secondary. 

It's a little different.
I'm trying to validate the checksum according to '6.3 Generic Primary DirectoryEntry Template'.

> In addition, trying add the checksum check.
> Conversely, Why would you want to add those checks to exfat_get_dentry_set()?

We should validate the EntrySet before using its contents.
(should not use contents of the EntrySet without validating it)
There are other filesystem designs that include crc/checksum in their each metadata headers.
Such a design can detect inconsistencies easily and effectively.
This verification is especially effective when meta-data is recorded in multiple sectors like the EntrySet.

> Why do we check only name entries separately? 

This verification is according to the description in '7.6.3 NameLength Field' and '7.7 File Name Directory Entry'.
Separated because it is  according to different description from checksum.
As I wrote before, I think TYPE_NAME and NameLength validation should not be separated from the name extraction.
(Because they are more strongly related than order of dir-entries)
So I think these should not be mixed with checksum verification.

When packing names into Name Dir-Entry...
We can also calculate the checksum while packing the name into the Name Dir-Entry.
However, we shouldn't mix them.


> Aren't you intent to return validated entry set in exfat_get_dentry_set()?

If so, add exfat_get_uniname_from_name_entries() after checksum verification.(as below)
----------------------------------------------------
	/* EntrySet checksum verification */

+	if (max_entries == ES_ALL_ENTRIES &&
+	    exfat_get_uniname_from_name_entries(es, es->uniname))
+		goto free_es;
	return es;
----------------------------------------------------

The only callers that need a name are exfat_readdir() and exfat_find_dir_entry(), not the others.
Unnecessary name extraction violates the JIT principle.
(The size of exfat_entry_set_cache will also increase)
And even if we call exfat_get_uniname_from_name_entries() later, we can correctly determine 
"File Name directory entries are valid only if they immediately follow the Stream Extension directory entry as a consecutive series"

File dir-entry set can contain dir-entries other than file, stream-ext and name.
We don't need to validate or extract them all in exfat_get_dentry_set().
I think exfat_entry_set_cache only needs to provide a framework for easy access when needed.

BR
---
Kohada Tetsuhiro <Kohada.Tetsuhiro@...MitsubishiElectric.co.jp>

Powered by blists - more mailing lists