linux-kernel - Re: [RFC PATCH] vfat:avoid unnecessary check

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]

Message-ID: <87plc4n9e5.fsf@mail.parknet.co.jp>
Date: Sat, 06 Sep 2025 01:37:06 +0900
From: OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
To: zhoumin <teczm@...mail.com>
Cc: linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] vfat:avoid unnecessary check

zhoumin <teczm@...mail.com> writes:

>>> Remove redundant and unreachable name check code in dir.c.
>
>> Looks like you changed the logic, but no explanation.
>
> 1. In fat_parse_long:
> If (*de)->name[0] equals DELETED_FLAG, the function returns immediately.
> Consequently, the subsequent IS_FREE check can never evaluate to true.
> Therefore, retaining only the ATTR_VOLUME verification should be sufficient.
>
> 2. In fat_search_long:
> If (*de)->name[0] equals DELETED_FLAG, the loop skips to the next iteration.
> This makes the subsequent checks for IS_FREE and ATTR_EXT unreachable.These
> checks should therefore be removed.
>
> 3. In __fat_readdir:
> The same reasoning as in fat_search_long applies here.

Hm, IS_FREE() checks 0 and DELETED_FLAG, isn't it?

>>> Remove flags check in fat_update_time since fat does not support
>>> inode version.
>>>
>>> Optimize fat_truncate_time to return a meaningful value, allowing
>>> the removal of redundant inode checks in fat_update_time. This
>>> ensures non-root inodes are validated only once.
>
>> Also changed the logic, you removed the check of flags.
>
> Changing the return value of fat_truncate_time and removing the ino check in 
> fat_update_time is a minor optimization, as mentioned in my previous patch email.
>
> The reason for removing the flags check is that the enum file_time_flags has
> only four values. Since vfat does not support SB_I_VERSION, higher-level
> functions such as inode_needs_update_time or inode_update_timestamps will never
> set flags with S_VERSION. Thus, checking the flags is unnecessary.
>
> Note that __mark_inode_dirty will not be called only for the root inode. This
> logic remains consistent with the previous version.

OK, thanks. I got the reason.

However I would prefer to keep the current code, for readability and
future changes, and explicitly check those time flags if there is no
measurable improvement.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>