linux-ext4 - Re: [PATCH 1/2] fs: dcache: Handle case-exact lookup in d_alloc

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]

Date:   Fri, 01 Oct 2021 14:35:32 -0400
From:   Gabriel Krisman Bertazi <krisman@...labora.com>
To:     Shreeya Patel <shreeya.patel@...labora.com>
Cc:     tytso@....edu, viro@...iv.linux.org.uk, adilger.kernel@...ger.ca,
        linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel@...labora.com
Subject: Re: [PATCH 1/2] fs: dcache: Handle case-exact lookup in
 d_alloc_parallel

Shreeya Patel <shreeya.patel@...labora.com> writes:

> There is a soft hang caused by a deadlock in d_alloc_parallel which
> waits up on lookups to finish for the dentries in the parent directory's
> hash_table.
> In case when d_add_ci is called from the fs layer's lookup functions,
> the dentry being looked up is already in the hash table (created before
> the fs lookup function gets called). We should not be processing the
> same dentry that is being looked up, hence, in case of case-insensitive
> filesystems we are making it a case-exact match to prevent this from
> happening.
>
> Signed-off-by: Shreeya Patel <shreeya.patel@...labora.com>
> ---
>  fs/dcache.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index cf871a81f4fd..2a28ab64a165 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -2565,6 +2565,15 @@ static void d_wait_lookup(struct dentry *dentry)
>  	}
>  }
>  
> +static inline bool d_same_exact_name(const struct dentry *dentry,
> +				     const struct dentry *parent,
> +				     const struct qstr *name)
> +{
> +	if (dentry->d_name.len != name->len)
> +		return false;
> +	return dentry_cmp(dentry, name->name, name->len) == 0;
> +}

I don't like the idea of having a flavor of a dentry comparison function
that doesn't invoke d_compare.  In particular because d_compare might be
used for all sorts of things, and this fix is really specific to the
case-insensitive case.

Would it be possible to fold this change into generic_ci_d_compare?  If
we could flag the dentry as part of a parallel lookup under the relevant
condition, generic_ci_d_compare could simply return immediately in
such case.

> +
>  struct dentry *d_alloc_parallel(struct dentry *parent,
>  				const struct qstr *name,
>  				wait_queue_head_t *wq)
> @@ -2575,6 +2584,7 @@ struct dentry *d_alloc_parallel(struct dentry *parent,
>  	struct dentry *new = d_alloc(parent, name);
>  	struct dentry *dentry;
>  	unsigned seq, r_seq, d_seq;
> +	int ci_dir = IS_CASEFOLDED(parent->d_inode);
>  
>  	if (unlikely(!new))
>  		return ERR_PTR(-ENOMEM);
> @@ -2626,8 +2636,14 @@ struct dentry *d_alloc_parallel(struct dentry *parent,
>  			continue;
>  		if (dentry->d_parent != parent)
>  			continue;
> -		if (!d_same_name(dentry, parent, name))
> -			continue;
> +		if (ci_dir) {
> +			if (!d_same_exact_name(dentry, parent, name))
> +				continue;
> +		} else {


As is, this is problematic because d_alloc_parallel is also part of the
lookup path (see lookup_open, lookup_slow).  In those cases, you want to
do the CI comparison, to prevent racing two tasks creating a dentry
differing only by case.


> +			if (!d_same_name(dentry, parent, name))
> +				continue;
> +		}
> +
>  		hlist_bl_unlock(b);
>  		/* now we can try to grab a reference */
>  		if (!lockref_get_not_dead(&dentry->d_lockref)) {

-- 
Gabriel Krisman Bertazi