| 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
| ||
|
Message-ID: <22a794e8-39c1-4f30-80c4-989a81c6b968@igalia.com>
Date: Thu, 14 Aug 2025 10:02:17 -0300
From: André Almeida <andrealmeid@...lia.com>
To: Amir Goldstein <amir73il@...il.com>
Cc: Miklos Szeredi <miklos@...redi.hu>, Theodore Tso <tytso@....edu>,
Gabriel Krisman Bertazi <krisman@...nel.org>, linux-unionfs@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
Alexander Viro <viro@...iv.linux.org.uk>,
Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
kernel-dev@...lia.com
Subject: Re: [PATCH v4 3/9] ovl: Create ovl_casefold() to support casefolded
strncmp()
Hi Amir,
Em 14/08/2025 09:53, Amir Goldstein escreveu:
> On Thu, Aug 14, 2025 at 12:37 AM André Almeida <andrealmeid@...lia.com> wrote:
>>
>> To add overlayfs support casefold filesystems, create a new function
>> ovl_casefold(), to be able to do case-insensitive strncmp().
>>
>> ovl_casefold() allocates a new buffer and stores the casefolded version
>> of the string on it. If the allocation or the casefold operation fails,
>> fallback to use the original string.
>>
>> The case-insentive name is then used in the rb-tree search/insertion
>> operation. If the name is found in the rb-tree, the name can be
>> discarded and the buffer is freed. If the name isn't found, it's then
>> stored at struct ovl_cache_entry to be used later.
>>
>> Signed-off-by: André Almeida <andrealmeid@...lia.com>
>> ---
[...]
>> + }
>>
>> INIT_LIST_HEAD(list);
>> }
>> @@ -260,12 +311,28 @@ static bool ovl_fill_merge(struct dir_context *ctx, const char *name,
>> {
>> struct ovl_readdir_data *rdd =
>> container_of(ctx, struct ovl_readdir_data, ctx);
>> + struct ovl_fs *ofs = OVL_FS(rdd->dentry->d_sb);
>> + const char *aux = NULL;
>
> It looks strange to me that you need aux
> and it looks strange to pair <aux, cf_len>
> neither here or there...
>
The reason behind this `aux` var is because I need a `const char`
pointer to point to the `name` argument, and `cf_name` can't be const
because it goes through ovl_casefold(). I tried a couple approaches here
to get rid of the compiler warning regarding const, and the only way I
managed to was using a third variable like that.
>> + char *cf_name = NULL;
>> + int cf_len = 0;
>> +
>> + if (ofs->casefold)
>> + cf_len = ovl_casefold(rdd->map, name, namelen, &cf_name);
>> +
>> + if (cf_len <= 0) {
>> + aux = name;
>
> why not:
> cf_name = name;
>
>> + cf_len = namelen;
>> + } else {
>> + aux = cf_name;
>> + }
>
> and no aux and no else needed at all?
>
> If you don't like a var named cf_name to point at a non-casefolded
> name buffer, then use other var names which are consistent such as
> <c_name, c_len> (c for "canonical" or "compare" name).
>
>>
>> rdd->count++;
>> if (!rdd->is_lowest)
>> - return ovl_cache_entry_add_rb(rdd, name, namelen, ino, d_type);
>> + return ovl_cache_entry_add_rb(rdd, name, namelen, aux, cf_len,
>> + ino, d_type);
>> else
>> - return ovl_fill_lowest(rdd, name, namelen, offset, ino, d_type);
>> + return ovl_fill_lowest(rdd, name, namelen, aux, cf_len,
>> + offset, ino, d_type);
>> }
>>
>
> What do you think about moving all the consume/free buffer logic out to caller:
>
That looks way cleaner to me, thanks! I will apply this approach for v5.
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index b65cdfce31ce..e77530c63207 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -174,7 +174,8 @@ static struct ovl_cache_entry
> *ovl_cache_entry_new(struct ovl_readdir_data *rdd,
> return p;
> }
>
> -static bool ovl_cache_entry_add_rb(struct ovl_readdir_data *rdd,
> +/* Return 0 for found, >0 for added, <0 for error */
> +static int ovl_cache_entry_add_rb(struct ovl_readdir_data *rdd,
> const char *name, int len, u64 ino,
> unsigned int d_type)
> {
> @@ -183,22 +184,23 @@ static bool ovl_cache_entry_add_rb(struct
> ovl_readdir_data *rdd,
> struct ovl_cache_entry *p;
>
> if (ovl_cache_entry_find_link(name, len, &newp, &parent))
> - return true;
> + return 0;
>
> p = ovl_cache_entry_new(rdd, name, len, ino, d_type);
> if (p == NULL) {
> rdd->err = -ENOMEM;
> - return false;
> + return -ENOMEM;
> }
>
> list_add_tail(&p->l_node, rdd->list);
> rb_link_node(&p->node, parent, newp);
> rb_insert_color(&p->node, rdd->root);
>
> - return true;
> + return 1;
> }
>
> -static bool ovl_fill_lowest(struct ovl_readdir_data *rdd,
> +/* Return 0 for found, >0 for added, <0 for error */
> +static int ovl_fill_lowest(struct ovl_readdir_data *rdd,
> const char *name, int namelen,
> loff_t offset, u64 ino, unsigned int d_type)
> {
> @@ -207,6 +209,7 @@ static bool ovl_fill_lowest(struct ovl_readdir_data *rdd,
> p = ovl_cache_entry_find(rdd->root, name, namelen);
> if (p) {
> list_move_tail(&p->l_node, &rdd->middle);
> + return 0;
> } else {
> p = ovl_cache_entry_new(rdd, name, namelen, ino, d_type);
> if (p == NULL)
> @@ -215,7 +218,7 @@ static bool ovl_fill_lowest(struct ovl_readdir_data *rdd,
> list_add_tail(&p->l_node, &rdd->middle);
> }
>
> - return rdd->err == 0;
> + return rdd->err ?: 1;
> }
>
> @@ -260,12 +263,31 @@ static bool ovl_fill_merge(struct dir_context
> *ctx, const char *name,
> {
> struct ovl_readdir_data *rdd =
> container_of(ctx, struct ovl_readdir_data, ctx);
> + struct ovl_fs *ofs = OVL_FS(rdd->dentry->d_sb);
> + char *c_name = NULL;
> + int c_len = 0;
> + int ret;
> +
> + if (ofs->casefold)
> + c_len = ovl_casefold(rdd->map, name, namelen, &c_name);
> +
> + if (c_len <= 0) {
> + c_name = name;
> + c_len = namelen;
> + }
>
> rdd->count++;
> - if (!rdd->is_lowest)
> - return ovl_cache_entry_add_rb(rdd, name, namelen, ino, d_type);
> - else
> - return ovl_fill_lowest(rdd, name, namelen, offset, ino, d_type);
> + if (!rdd->is_lowest) {
> + ret = ovl_cache_entry_add_rb(rdd, name, namelen, c_name, c_len,
> + ino, d_type);
> + } else {
> + ret = ovl_fill_lowest(rdd, name, namelen, c_name, c_len, offset,
> + ino, d_type);
> + }
> + // ret > 1 means c_name is consumed
> + if (ret <= 0 && c_len > 0)
> + kfree(c_name);
> + return ret >= 0;
> }
>
> Thanks,
> Amir.
Powered by blists - more mailing lists