linux-ext4 - Re: [PATCH 1/5] ext4: Match the f2fs ci

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: <YkJ1xmB1wXQ1WCJ7@sol.localdomain>
Date:   Mon, 28 Mar 2022 19:58:14 -0700
From:   Eric Biggers <ebiggers@...nel.org>
To:     Gabriel Krisman Bertazi <krisman@...labora.com>
Cc:     tytso@....edu, jaegeuk@...nel.org, linux-ext4@...r.kernel.org,
        kernel@...labora.com
Subject: Re: [PATCH 1/5] ext4: Match the f2fs ci_compare implementation

On Mon, Mar 21, 2022 at 11:00:00PM -0400, Gabriel Krisman Bertazi wrote:
> ext4_ci_compare originally follows utf8_*_strcmp, which means return
> zero on match.  This means that every usage of that in ext4 negates
> the return.
> 
> Turn it into a predicate function, let it follow the kernel convention
> and return true on match, which means it's now the same as its f2fs
> counterpart and can be extracted into generic code.
> 
> This change also makes it more obvious that we are ignoring error
> handling in ext4_match, which can occur since casefolding support (bad
> utf8 name due to disk corruption on strict mode causes -EINVAL) and
> casefold+encryption (-ENOMEM).  For now, keep the behavior.  It is
> handled by the following patches.
> 
> While we are there, change the comment to the kernel-doc style.
> 
> Signed-off-by: Gabriel Krisman Bertazi <krisman@...labora.com>
> ---
>  fs/ext4/namei.c | 62 +++++++++++++++++++++++++++++++++----------------
>  1 file changed, 42 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 8cf0a924a49b..24ea3bb446d0 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -1318,13 +1318,20 @@ static void dx_insert_block(struct dx_frame *frame, u32 hash, ext4_lblk_t block)
>  }
>  
>  #if IS_ENABLED(CONFIG_UNICODE)
> -/*
> +/**
> + * ext4_ci_compare() - Match (case-insensitive) a name with a dirent.
> + * @parent: Inode of the parent of the dentry.
> + * @name: name under lookup.
> + * @de_name: Dirent name.
> + * @de_name_len: dirent name length.
> + * @quick: whether @name is already casefolded.
> + *
>   * Test whether a case-insensitive directory entry matches the filename
> - * being searched for.  If quick is set, assume the name being looked up
> - * is already in the casefolded form.
> + * being searched.  If quick is set, the @name being looked up is
> + * already in the casefolded form.
>   *
> - * Returns: 0 if the directory entry matches, more than 0 if it
> - * doesn't match or less than zero on error.
> + * Return: > 0 if the directory entry matches, 0 if it doesn't match, or
> + * < 0 on error.
>   */
>  static int ext4_ci_compare(const struct inode *parent, const struct qstr *name,
>  			   u8 *de_name, size_t de_name_len, bool quick)

Shouldn't this be renamed to ext4_match_ci() as well?  The f2fs equivalent is
called f2fs_match_ci_name(), and this is called from ext4_match().
ext4_match_ci() would better fit the "return 1 on match" behavior, I think.

- Eric