[PATCH 0/2] Handle a soft hang and the inconsistent name issue

[PATCH 0/2] Handle a soft hang and the inconsistent name issue

[Shreeya Patel]

When d_add_ci is called from the fs layer, we face a soft hang which is
caused by the deadlock in d_alloc_parallel. First patch in the series
tries to resolve it by doing a case-exact match instead of the
case-inexact match done by d_same_name function.

The second patch resolves the inconsistent name that is exposed by
/proc/self/cwd in case of a case-insensitive filesystem.
/proc/self/cwd uses the dentry name stored in dcache. Since the dcache
is populated only on the first lookup, with the string used in that lookup,
cwd will have an unexpected case, depending on how the data was first
looked-up in a case-insesitive filesystem.


Shreeya Patel (2):
  fs: dcache: Handle case-exact lookup in d_alloc_parallel
  fs: ext4: Fix the inconsistent name exposed by /proc/self/cwd

 fs/dcache.c     | 20 ++++++++++++++++++--
 fs/ext4/namei.c | 13 +++++++++++++
 2 files changed, 31 insertions(+), 2 deletions(-)

-- 
2.30.2

[PATCH 1/2] fs: dcache: Handle case-exact lookup in d_alloc_parallel

[Shreeya Patel]

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@collabora.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;
+}
+
 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 {
+			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)) {
-- 
2.30.2

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

[Gabriel Krisman Bertazi]

Shreeya Patel <shreeya.patel@collabora.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@collabora.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

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

[Al Viro]

On Fri, Oct 01, 2021 at 02:35:32PM -0400, Gabriel Krisman Bertazi wrote:

> 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.

Not really - if anything, that's a property of d_alloc_parallel() call
done by d_add_ci(), not that of any of the dentries involved...

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

[Al Viro]

On Wed, Sep 29, 2021 at 04:23:38PM +0530, Shreeya Patel wrote:
> 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.

NAK.  What you are doing would lead to parallel calls of ->lookup() in the
same directory for names that would compare as equal.  Which violates
all kinds of assumptions in the analysis of dentry tree locking.

d_add_ci() is used to force the "exact" spelling of the name on lookup -
that's the whole point of that thing.  What are you trying to achieve,
and what's the point of mixing that with non-trivial ->d_compare()?

If it's "force to exact spelling on lookup, avoid calling ->lookup() on
aliases", d_add_ci() is simply not a good match.

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

[Shreeya Patel]

On 03/10/21 7:08 pm, Al Viro wrote:
> On Wed, Sep 29, 2021 at 04:23:38PM +0530, Shreeya Patel wrote:
>> 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.
> NAK.  What you are doing would lead to parallel calls of ->lookup() in the
> same directory for names that would compare as equal.  Which violates
> all kinds of assumptions in the analysis of dentry tree locking.
>
> d_add_ci() is used to force the "exact" spelling of the name on lookup -
> that's the whole point of that thing.  What are you trying to achieve,
> and what's the point of mixing that with non-trivial ->d_compare()?
>
Sending again as plain text...

Hi Al Viro,

This patch was added to resolve some of the issues faced in patch 02/02 
of the series.

Originally, the 'native', per-directory case-insensitive implementation
merged in ext4/f2fs stores the case of the first lookup on the dcache,
regardless of the disk exact file name case. This gets reflected in symlink
returned by /proc/self/cwd.

To solve this we are calling d_add_ci from the fs lookup function to 
store the
disk exact name in the dcache even if an inexact-match string is used on 
the FIRST lookup.
But this caused a soft hang since there was a deadlock in d_wait_lookup 
called from d_alloc_parallel.

The reason for the hang is that d_same_name uses d_compare which does a
case-insensitive match and is able to find the dentry name in the 
secondary hash table
leading it to d_wait_lookup which would wait for the lookup to finish on 
that dentry
causing a deadlock.

To avoid the hang, we are doing a case-sensitive match using dentry_cmp 
here.


Thanks

> If it's "force to exact spelling on lookup, avoid calling ->lookup() on
> aliases", d_add_ci() is simply not a good match.

[PATCH 2/2] fs: ext4: Fix the inconsistent name exposed by /proc/self/cwd

[Shreeya Patel]

/proc/self/cwd is a symlink created by the kernel that uses whatever
name the dentry has in the dcache. Since the dcache is populated only
on the first lookup, with the string used in that lookup, cwd will
have an unexpected case, depending on how the data was first looked-up
in a case-insesitive filesystem.

Steps to reproduce :-

root@test-box:/src# mkdir insensitive/foo
root@test-box:/src# cd insensitive/FOO
root@test-box:/src/insensitive/FOO# ls -l /proc/self/cwd
lrwxrwxrwx 1 root root /proc/self/cwd -> /src/insensitive/FOO

root@test-box:/src/insensitive/FOO# cd ../fOo
root@test-box:/src/insensitive/fOo# ls -l /proc/self/cwd
lrwxrwxrwx 1 root root /proc/self/cwd -> /src/insensitive/FOO

Above example shows that 'FOO' was the name used on first lookup here and
it is stored in dcache instead of the original name 'foo'. This results
in inconsistent name exposed by /proc/self/cwd since it uses the name
stored in dcache.

To avoid the above inconsistent name issue, handle the inexact-match string
( a string which is not a byte to byte match, but is an equivalent
unicode string ) case in ext4_lookup which would store the original name
in dcache using d_add_ci instead of the inexact-match string name.

Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
---
 fs/ext4/namei.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index da7698341d7d..3598f0e47067 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1801,6 +1801,19 @@ static struct dentry *ext4_lookup(struct inode *dir, struct dentry *dentry, unsi
 	}
 
 #ifdef CONFIG_UNICODE
+	if (inode && IS_CASEFOLDED(dir))
+		if (dentry && strcmp(dentry->d_name.name, de->name)) {
+			struct dentry *new;
+			struct qstr ciname;
+
+			ciname.len = de->name_len;
+			ciname.name = kstrndup(de->name, ciname.len, GFP_NOFS);
+			if (!ciname.name)
+				return ERR_PTR(-ENOMEM);
+			new = d_add_ci(dentry, inode, &ciname);
+			kfree(ciname.name);
+			return new;
+	}
 	if (!inode && IS_CASEFOLDED(dir)) {
 		/* Eventually we want to call d_add_ci(dentry, NULL)
 		 * for negative dentries in the encoding case as
-- 
2.30.2

Re: [PATCH 2/2] fs: ext4: Fix the inconsistent name exposed by /proc/self/cwd

[Theodore Ts'o]

On Wed, Sep 29, 2021 at 04:23:39PM +0530, Shreeya Patel wrote:
> /proc/self/cwd is a symlink created by the kernel that uses whatever
> name the dentry has in the dcache. Since the dcache is populated only
> on the first lookup, with the string used in that lookup, cwd will
> have an unexpected case, depending on how the data was first looked-up
> in a case-insesitive filesystem.
> 
> Steps to reproduce :-
> 
> root@test-box:/src# mkdir insensitive/foo
> root@test-box:/src# cd insensitive/FOO
> root@test-box:/src/insensitive/FOO# ls -l /proc/self/cwd
> lrwxrwxrwx 1 root root /proc/self/cwd -> /src/insensitive/FOO
> 
> root@test-box:/src/insensitive/FOO# cd ../fOo
> root@test-box:/src/insensitive/fOo# ls -l /proc/self/cwd
> lrwxrwxrwx 1 root root /proc/self/cwd -> /src/insensitive/FOO
> 
> Above example shows that 'FOO' was the name used on first lookup here and
> it is stored in dcache instead of the original name 'foo'. This results
> in inconsistent name exposed by /proc/self/cwd since it uses the name
> stored in dcache.
> 
> To avoid the above inconsistent name issue, handle the inexact-match string
> ( a string which is not a byte to byte match, but is an equivalent
> unicode string ) case in ext4_lookup which would store the original name
> in dcache using d_add_ci instead of the inexact-match string name.

I'm not sure this is a problem.  /proc/<pid>/cwd just needs to point
at the current working directory for the process.  Why do we care
whether it matches the case that was stored on disk?  Whether we use
/src/insensitive/FOO, or /src/insensitive/Foo, or
/src/insensitive/foo, all of these will reach the cwd for that
process.

					- Ted

Re: [PATCH 2/2] fs: ext4: Fix the inconsistent name exposed by /proc/self/cwd

[Gabriel Krisman Bertazi]

"Theodore Ts'o" <tytso@mit.edu> writes:

> On Wed, Sep 29, 2021 at 04:23:39PM +0530, Shreeya Patel wrote:
>> /proc/self/cwd is a symlink created by the kernel that uses whatever
>> name the dentry has in the dcache. Since the dcache is populated only
>> on the first lookup, with the string used in that lookup, cwd will
>> have an unexpected case, depending on how the data was first looked-up
>> in a case-insesitive filesystem.
>> 
>> Steps to reproduce :-
>> 
>> root@test-box:/src# mkdir insensitive/foo
>> root@test-box:/src# cd insensitive/FOO
>> root@test-box:/src/insensitive/FOO# ls -l /proc/self/cwd
>> lrwxrwxrwx 1 root root /proc/self/cwd -> /src/insensitive/FOO
>> 
>> root@test-box:/src/insensitive/FOO# cd ../fOo
>> root@test-box:/src/insensitive/fOo# ls -l /proc/self/cwd
>> lrwxrwxrwx 1 root root /proc/self/cwd -> /src/insensitive/FOO
>> 
>> Above example shows that 'FOO' was the name used on first lookup here and
>> it is stored in dcache instead of the original name 'foo'. This results
>> in inconsistent name exposed by /proc/self/cwd since it uses the name
>> stored in dcache.
>> 
>> To avoid the above inconsistent name issue, handle the inexact-match string
>> ( a string which is not a byte to byte match, but is an equivalent
>> unicode string ) case in ext4_lookup which would store the original name
>> in dcache using d_add_ci instead of the inexact-match string name.
>
> I'm not sure this is a problem.  /proc/<pid>/cwd just needs to point
> at the current working directory for the process.  Why do we care
> whether it matches the case that was stored on disk?  Whether we use
> /src/insensitive/FOO, or /src/insensitive/Foo, or
> /src/insensitive/foo, all of these will reach the cwd for that
> process.

Hi Ted,

The dcache name is exposed in more places, like /proc/mounts.  We have a
bug reported against flatpak where its initialization code bind mounts a
directory that was previously touched with a different case combination,
and then checks /proc/mounts in a case-sensitive way to see if the mount
succeeded.  This code now regresses on CI directories because the name
it asked to bind mount is not found in /proc/mounts.

Sure, we could figure out the dcache name and pass the current case
spelling of the directory to flatpak, but that could go away at any
time.  We could also make flatpak CI aware, but that problem will just
appear elsewhere.

I think the more reasonable approach is to save the disk exact name on
the dcache, because that is the only version that doesn't change based
on who won the race for the first lookup.

-- 
Gabriel Krisman Bertazi

Re: [PATCH 2/2] fs: ext4: Fix the inconsistent name exposed by /proc/self/cwd

[Theodore Ts'o]

On Fri, Oct 01, 2021 at 03:11:30PM -0400, Gabriel Krisman Bertazi wrote:
> 
> The dcache name is exposed in more places, like /proc/mounts.  We have a
> bug reported against flatpak where its initialization code bind mounts a
> directory that was previously touched with a different case combination,
> and then checks /proc/mounts in a case-sensitive way to see if the mount
> succeeded.  This code now regresses on CI directories because the name
> it asked to bind mount is not found in /proc/mounts.

Ah, thanks for the context.  That makes sense.

> I think the more reasonable approach is to save the disk exact name on
> the dcache, because that is the only version that doesn't change based
> on who won the race for the first lookup.

What about the alternative of storing the casefolded name?  The
advantage of using the casefolded name is that we can always casefold
the name, where as in the case of a negative dentry, there is no disk
exact name to use (since by definition there is no on-disk name).

      	      	  	    	       - Ted

Re: [PATCH 2/2] fs: ext4: Fix the inconsistent name exposed by /proc/self/cwd

[Gabriel Krisman Bertazi]

"Theodore Ts'o" <tytso@mit.edu> writes:

> On Fri, Oct 01, 2021 at 03:11:30PM -0400, Gabriel Krisman Bertazi wrote:
>> 
>> The dcache name is exposed in more places, like /proc/mounts.  We have a
>> bug reported against flatpak where its initialization code bind mounts a
>> directory that was previously touched with a different case combination,
>> and then checks /proc/mounts in a case-sensitive way to see if the mount
>> succeeded.  This code now regresses on CI directories because the name
>> it asked to bind mount is not found in /proc/mounts.
>
> Ah, thanks for the context.  That makes sense.
>
>> I think the more reasonable approach is to save the disk exact name on
>> the dcache, because that is the only version that doesn't change based
>> on who won the race for the first lookup.
>
> What about the alternative of storing the casefolded name?  The
> advantage of using the casefolded name is that we can always casefold
> the name, where as in the case of a negative dentry, there is no disk
> exact name to use (since by definition there is no on-disk name).

That would work.  The casefolded version is always predictable (since
unicode is stable) and even though is not as easily available as the
disk name function (getdents), it solves the issue.

It would also allow us to use utf8_strncasecmp_folded in the d_compare
hook, which is nice.

Do you have an implementation suggestion to solve the dcache issue
pointed by Viro?

-- 
Gabriel Krisman Bertazi

Re: [PATCH 0/2] Handle a soft hang and the inconsistent name issue

[Gabriel Krisman Bertazi]

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

> When d_add_ci is called from the fs layer, we face a soft hang which is
> caused by the deadlock in d_alloc_parallel. First patch in the series
> tries to resolve it by doing a case-exact match instead of the
> case-inexact match done by d_same_name function.

Hi Shreeya,

I understand what you are trying to solve here, but this could use some
clarification.

There is no such deadlock in the upstream code base, since d_add_ci is
never called by a file system with a d_compare hook that causes the
issue.  Patch 02/02 will be the first to include such path, to address
the /proc/self/cwd leakage, therefore, Patch 01/02 is done in
preparation of that patch.  That needs to be clearly stated here.

Originally, the 'native', per-directory case-insensitive implementation
merged in ext4/f2fs stores the case of the first lookup on the dcache,
regardless of the disk exact file name case.  This is intended as an
internal implementation detail, that shouldn't be leaked to
userspace. Whenever the kernel returns a name to userspace it should be
the exact name, as written on disk.  But, on /proc/self/cwd, the
internal name is leaked to userspace.  The goal of the series is
*solely* to fix the leakage of this implementation detail to userspace.

I think the solution is in the right direction, but I see some
issues on the implementation I'm discussing inline.

> The second patch resolves the inconsistent name that is exposed by
>/proc/self/cwd in case of a case-insensitive filesystem.
>/proc/self/cwd uses the dentry name stored in dcache. Since the dcache
>is populated only on the first lookup, with the string used in that
>lookup, cwd will have an unexpected case, depending on how the data was
>first looked-up in a case-insesitive filesystem.
>
>
> Shreeya Patel (2):
>   fs: dcache: Handle case-exact lookup in d_alloc_parallel
>   fs: ext4: Fix the inconsistent name exposed by /proc/self/cwd
>
>  fs/dcache.c     | 20 ++++++++++++++++++--
>  fs/ext4/namei.c | 13 +++++++++++++
>  2 files changed, 31 insertions(+), 2 deletions(-)

-- 
Gabriel Krisman Bertazi