linux-kernel - RE: [PATCH 2/2] exfat: unify name extraction

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 21 Aug 2020 19:41:38 +0900
From:   "Sungjong Seo" <sj1557.seo@...sung.com>
To:     "'Tetsuhiro Kohada'" <kohada.t2@...il.com>
Cc:     <kohada.tetsuhiro@...mitsubishielectric.co.jp>,
        <mori.takahiro@...mitsubishielectric.co.jp>,
        <motai.hirotaka@...mitsubishielectric.co.jp>,
        "'Namjae Jeon'" <namjae.jeon@...sung.com>,
        <linux-fsdevel@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 2/2] exfat: unify name extraction

> Thanks for your reply.
> 
> On 2020/08/09 2:19, Sungjong Seo wrote:
> > [snip]
> >> @@ -963,80 +942,38 @@ int exfat_find_dir_entry(struct super_block
> >> *sb, struct exfat_inode_info *ei,
> >>   			num_empty = 0;
> >>   			candi_empty.eidx = EXFAT_HINT_NONE;
> >>
> > [snip]
> >>
> >> -			if (entry_type &
> >> -					(TYPE_CRITICAL_SEC |
> > TYPE_BENIGN_SEC)) {
> >> -				if (step == DIRENT_STEP_SECD) {
> >> -					if (++order == num_ext)
> >> -						goto found;
> >> -					continue;
> >> -				}
> >> +			exfat_get_uniname_from_name_entries(es, &uni_name);
> >
> > It is needed to check a return value.
> 
> I'll fix it in v2.
> 
> 
> >> +			exfat_free_dentry_set(es, false);
> >> +
> >> +			if (!exfat_uniname_ncmp(sb,
> >> +						p_uniname->name,
> >> +						uni_name.name,
> >> +						name_len)) {
> >> +				/* set the last used position as hint */
> >> +				hint_stat->clu = clu.dir;
> >> +				hint_stat->eidx = dentry;
> >
> > eidx and clu of hint_stat should have one for the next entry we'll
> > start looking for.
> > Did you intentionally change the concept?
> 
> Yes, this is intentional.
> Essentially, the "Hint" concept is to reduce the next seek cost with
> minimal cost.
> There is a difference in the position of the hint, but the concept is the
> same.
> As you can see, the patched code strategy doesn't move from current
> position.
> Basically, the original code strategy is advancing only one dentry.(It's
> the "minimum cost") However, when it reaches the cluster boundary, it gets
> the next cluster and error handling.

I didn't get exactly what "original code" is.
Do you mean whole code lines for exfat_find_dir_entry()?
Or just only for handling the hint in it?

The strategy of original code for hint is advancing not one dentry but one dentry_set.
If a hint position is not moved to next like the patched code,
caller have to start at old dentry_set that could be already loaded on dentry cache.

Let's think the case of searching through all files sequentially.
The patched code should check twice per a file.
No better than the original policy.

> Getting the next cluster The error handling already exists at the end of
> the while loop, so the code is duplicated.
> These costs should be paid next time and are no longer the "minimum cost".

I agree with your words, "These costs should be paid next time".
If so, how about moving the cluster handling for a hint dentry to
the beginning of the function while keeping the original policy?

BTW, this patch is not related to the hint code.
I think it would be better to keep the original code in this patch and improve it with a separate patch.

> Should I add this to the commit-message?
> 
> 
> BR
> ---
> Tetsuhiro Kohada <kohada.t2@...il.com>