Re: [PATCH v6 9/9] ovl: Support mounting case-insensitive enabled layers

["André Almeida" <andrealmeid@igalia.com>]

Em 27/08/2025 15:06, Amir Goldstein escreveu:

[...]

> 
> The reason is this:
> 
> static struct dentry *ext4_lookup(...
> {
> ...
>          if (IS_ENABLED(CONFIG_UNICODE) && !inode && IS_CASEFOLDED(dir)) {
>                  /* Eventually we want to call d_add_ci(dentry, NULL)
>                   * for negative dentries in the encoding case as
>                   * well.  For now, prevent the negative dentry
>                   * from being cached.
>                   */
>                  return NULL;
>          }
> 
>          return d_splice_alias(inode, dentry);
> }
> 
> Neil,
> 
> Apparently, the assumption that
> ovl_lookup_temp() => ovl_lookup_upper() => lookup_one()
> returns a hashed dentry is not always true.
> 
> It may be always true for all the filesystems that are currently
> supported as an overlayfs
> upper layer fs (?), but it does not look like you can count on this
> for the wider vfs effort
> and we should try to come up with a solution for ovl_parent_lock()
> that will allow enabling
> casefolding on overlayfs layers.
> 
> This patch seems to work. WDYT?
> 
> Thanks,
> Amir.
> 

Thank you for the fix!

> commit 5dfcd10378038637648f3f422e3d5097eb6faa5f
> Author: Amir Goldstein <amir73il@gmail.com>
> Date:   Wed Aug 27 19:55:26 2025 +0200
> 
>      ovl: adapt ovl_parent_lock() to casefolded directories
> 
>      e8bd877fb76bb9f3 ("ovl: fix possible double unlink") added a sanity

Just to make checkpatch happy, this should be

Commit e8bd877fb76b ("ovl: fix possible double unlink") added a sanity

>      check of !d_unhashed(child) to try to verify that child dentry was not
>      unlinked while parent dir was unlocked.
> 
>      This "was not unlink" check has a false positive result in the case of
>      casefolded parent dir, because in that case, ovl_create_temp() returns
>      an unhashed dentry.
> 
>      Change the "was not unlinked" check to use cant_mount(child).
>      cant_mount(child) means that child was unlinked while we have been
>      holding a reference to child, so it could not have become negative.
> 
>      This fixes the error in ovl_parent_lock() in ovl_check_rename_whiteout()
>      after ovl_create_temp() and allows mount of overlayfs with casefolding
>      enabled layers.
> 
>      Reported-by: André Almeida <andrealmeid@igalia.com>
>      Link: https://lore.kernel.org/r/18704e8c-c734-43f3-bc7c-b8be345e1bf5@igalia.com/

I think the correct chain here is:

Reported-by: André Almeida <andrealmeid@igalia.com>
Closes: 
https://lore.kernel.org/r/18704e8c-c734-43f3-bc7c-b8be345e1bf5@igalia.com/
Fixes: e8bd877fb76b ("ovl: fix possible double unlink")

>      Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> 

Reviewed-by: André Almeida <andrealmeid@igalia.com>

> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index bec4a39d1b97c..bffbb59776720 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -1551,9 +1551,23 @@ void ovl_copyattr(struct inode *inode)
> 
>   int ovl_parent_lock(struct dentry *parent, struct dentry *child)
>   {
> +       bool is_unlinked;
> +
>          inode_lock_nested(parent->d_inode, I_MUTEX_PARENT);
> -       if (!child ||
> -           (!d_unhashed(child) && child->d_parent == parent))
> +       if (!child)
> +               return 0;
> +
> +       /*
> +        * After re-acquiring parent dir lock, verify that child was not moved
> +        * to another parent and that it was not unlinked. cant_mount() means
> +        * that child was unlinked while parent was unlocked. Since we are
> +        * holding a reference to child, it could not have become negative.
> +        * d_unhashed(child) is not a strong enough indication for unlinked,
> +        * because with casefolded parent dir, ovl_create_temp() returns an
> +        * unhashed dentry.
> +        */
> +       is_unlinked = cant_mount(child) || WARN_ON_ONCE(d_is_negative(child));
> +       if (!is_unlinked && child->d_parent == parent)
>                  return 0;
> 
>          inode_unlock(parent->d_inode);