Handle a soft hang and the inconsistent name issue

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 <[email protected]>

---
 fs/dcache.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

fs/dcache.c +0 -0

index cf871a81f4fd..2a28ab64a165 100644
@@ -2565,6 +2565,15 @@ static void d_wait_lookup(struct dentry *dentry)
 	}

fs/dcache.c +2 -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);

fs/dcache.c +4 -0

+			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)) {

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.

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.

Quoted 9 lines · 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.

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.

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

Quoted 22 lines · 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.

Quoted 4 lines

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.

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.

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.

Quoted 7 lines · Gabriel Krisman Bertazi

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.

Quoted 3 lines

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

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(-)