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

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

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

/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 <redacted>
---
 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

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

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.

On 03/10/21 7:08 pm, Al Viro wrote:

On Wed, Sep 29, 2021 at 04:23:38PM +0530, Shreeya Patel wrote:

quoted

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.