lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <871q9x2vwj.fsf@mailhost.krisman.be> Date: Wed, 31 Jan 2024 15:35:40 -0300 From: Gabriel Krisman Bertazi <krisman@...e.de> To: Eric Biggers <ebiggers@...nel.org> Cc: viro@...iv.linux.org.uk, jaegeuk@...nel.org, tytso@....edu, amir73il@...il.com, linux-ext4@...r.kernel.org, linux-f2fs-devel@...ts.sourceforge.net, linux-fsdevel@...r.kernel.org Subject: Re: [PATCH v5 04/12] fscrypt: Drop d_revalidate for valid dentries during lookup Eric Biggers <ebiggers@...nel.org> writes: > On Mon, Jan 29, 2024 at 05:43:22PM -0300, Gabriel Krisman Bertazi wrote: >> Unencrypted and encrypted-dentries where the key is available don't need >> to be revalidated with regards to fscrypt, since they don't go stale >> from under VFS and the key cannot be removed for the encrypted case >> without evicting the dentry. Mark them with d_set_always_valid, to > > "d_set_always_valid" doesn't appear in the diff itself. > >> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h >> index 4aaf847955c0..a22997b9f35c 100644 >> --- a/include/linux/fscrypt.h >> +++ b/include/linux/fscrypt.h >> @@ -942,11 +942,22 @@ static inline int fscrypt_prepare_rename(struct inode *old_dir, >> static inline void fscrypt_prepare_lookup_dentry(struct dentry *dentry, >> bool is_nokey_name) >> { >> - if (is_nokey_name) { >> - spin_lock(&dentry->d_lock); >> + spin_lock(&dentry->d_lock); >> + >> + if (is_nokey_name) >> dentry->d_flags |= DCACHE_NOKEY_NAME; >> - spin_unlock(&dentry->d_lock); >> + else if (dentry->d_flags & DCACHE_OP_REVALIDATE && >> + dentry->d_op->d_revalidate == fscrypt_d_revalidate) { >> + /* >> + * Unencrypted dentries and encrypted dentries where the >> + * key is available are always valid from fscrypt >> + * perspective. Avoid the cost of calling >> + * fscrypt_d_revalidate unnecessarily. >> + */ >> + dentry->d_flags &= ~DCACHE_OP_REVALIDATE; >> } >> + >> + spin_unlock(&dentry->d_lock); > > This makes lookups in unencrypted directories start doing the > spin_lock/spin_unlock pair. Is that really necessary? > > These changes also make the inline function fscrypt_prepare_lookup() very long > (when including the fscrypt_prepare_lookup_dentry() that's inlined into it). > The rule that I'm trying to follow is that to the extent that the fscrypt helper > functions are inlined, the inline part should be a fast path for unencrypted > directories. Encrypted directories should be handled out-of-line. > > So looking at the original fscrypt_prepare_lookup(): > > static inline int fscrypt_prepare_lookup(struct inode *dir, > struct dentry *dentry, > struct fscrypt_name *fname) > { > if (IS_ENCRYPTED(dir)) > return __fscrypt_prepare_lookup(dir, dentry, fname); > > memset(fname, 0, sizeof(*fname)); > fname->usr_fname = &dentry->d_name; > fname->disk_name.name = (unsigned char *)dentry->d_name.name; > fname->disk_name.len = dentry->d_name.len; > return 0; > } > > If you could just add the DCACHE_OP_REVALIDATE clearing for dentries in > unencrypted directories just before the "return 0;", hopefully without the > spinlock, that would be good. Yes, that does mean that > __fscrypt_prepare_lookup() will have to handle it too, for the case of dentries > in encrypted directories, but that seems okay. ok, will do. IIUC, we might be able to do without the d_lock provided there is no store tearing. But what was the reason you need the d_lock to set DCACHE_NOKEY_NAME during lookup? Is there a race with parallel lookup setting d_flag that I couldn't find? Or is it another reason? -- Gabriel Krisman Bertazi
Powered by blists - more mailing lists