Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Gabriel Krisman Bertazi <krisman@collabora.com>
To:     Eric Biggers <ebiggers@kernel.org>
Cc:     tytso@mit.edu, adilger.kernel@dilger.ca, jaegeuk@kernel.org,
        linux-ext4@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net,
        kernel@collabora.com
Subject: Re: [PATCH v6 4/8] ext4: Reuse generic_ci_match for ci comparisons
Organization: Collabora
References: <20220519014044.508099-1-krisman@collabora.com>
        <20220519014044.508099-5-krisman@collabora.com>
        <YoW8yx9Fw9Rwiaja@sol.localdomain>
Date:   Thu, 19 May 2022 15:52:10 -0400
In-Reply-To: <YoW8yx9Fw9Rwiaja@sol.localdomain> (Eric Biggers's message of
        "Wed, 18 May 2022 20:43:07 -0700")
Message-ID: <87h75lnvv9.fsf@collabora.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Eric Biggers <ebiggers@kernel.org> writes:

> On Wed, May 18, 2022 at 09:40:40PM -0400, Gabriel Krisman Bertazi wrote:
>> Instead of reimplementing ext4_match_ci, use the new libfs helper.
>> 
>> It should be fine to drop the fname->cf_name in the encrypted directory
>> case for the hash verification optimization because the only two ways
>> for fname->cf_name to be NULL on a case-insensitive lookup is
>> 
>>  (1) if name under lookup has an invalid encoding and the FS is not in
>>  strict mode; or
>> 
>>  (2) if the directory is encrypted and we don't have the
>>  key.
>> 
>> For case (1), it doesn't matter, because the lookup hash will be
>> generated with fname->usr_name, the same as the disk (fallback to
>> invalid encoding behavior on !strict mode).  Case (2) is caught by the
>> previous check (!IS_ENCRYPTED(parent) ||
>> fscrypt_has_encryption_key(parent)), so we never reach this code.
>
> The code actually can be reached in case (2), because the key could have been
> added between ext4_fname_setup_ci_filename() and ext4_match().

Hm, I see! I didn't understand it would be possible to add a key during
a lookup from your previous explanation, thanks for clarifying.

> I *think* your change doesn't make it any worse, since in such a case the name
> comparison is going to be comparing a no-key name to a regular one, which will
> very likely fail.  So adding an additional way for the match to fail
> seems fine.

Either way, no point in setting it for failure.  I will restore the
fname->cf_name != NULL check.

> It's hard to reason about, though.  f2fs does things in a much cleaner way, as
> I've mentioned before, since it decides which type of match it wants at the
> beginning, when initializing struct f2fs_filename.

Yes, this is quite confusing. Are these implementation documented
anywhere?

Thank you for the review!

-- 
Gabriel Krisman Bertazi