On Thu, Jul 02, 2020 at 06:01:37PM -0700, Daniel Rosenberg wrote: > On Tue, Jun 23, 2020 at 10:57 PM Eric Biggers <ebiggers@xxxxxxxxxx> wrote: > > > > 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
