On Thu, Jul 02, 2020 at 06:01:37PM -0700, Daniel Rosenberg wrote:

On Tue, Jun 23, 2020 at 10:57 PM Eric Biggers [off-list ref] wrote:

quoted

Note that the '!IS_ENCRYPTED(dir) || fscrypt_has_encryption_key(dir)' check can
be racy, because a process can be looking up a no-key token in a directory while
concurrently another process initializes the directory's ->i_crypt_info, causing
fscrypt_has_encryption_key(dir) to suddenly start returning true.

In my rework of filename handling in f2fs, I actually ended up removing all
calls to needs_casefold(), thus avoiding this race.  f2fs now decides whether
the name is going to need casefolding early on, in __f2fs_setup_filename(),
where it knows in a race-free way whether the filename is a no-key token or not.

Perhaps ext4 should work the same way?  It did look like there would be some
extra complexity due to how the ext4 directory hashing works in comparison to
f2fs's, but I haven't had a chance to properly investigate it.

- Eric

Hm. I think I should be able to just check for DCACHE_ENCRYPTED_NAME
in the dentry here, right? I'm just trying to avoid casefolding the
no-key token, and that flag should indicate that.

Ideally yes, but currently the 'struct dentry' isn't always available.  See how
fscrypt_setup_filename(), f2fs_setup_filename(), f2fs_find_entry(),
ext4_find_entry(), etc. take a 'struct qstr', not a 'struct dentry'.

At some point we should fix that by passing down the dentry whenever it's
available, so that we reliably know whether the name is a no-key name or not.

So even my new f2fs code is still racy.  But it at least handles each filename
in a consistent way within each directory operation.  In comparison, your
proposed ext4 code can treat a filename as a no-key name while matching one
dir_entry and then as a regular filename while matching the next.  I think the
f2fs way is more on the right track, both correctness-wise and efficiency-wise.

- Eric