Eric Biggers <ebiggers@xxxxxxxxxx> writes:
> On Mon, Feb 12, 2024 at 09:13:14PM -0500, Gabriel Krisman Bertazi wrote:
>> Finally, we need to clean the dentry->flags even for unencrypted
>> dentries, so the ->d_lock might be acquired even for them. In order to
>
> might => must?
>
>> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
>> index 47567a6a4f9d..d1f17b90c30f 100644
>> --- a/include/linux/fscrypt.h
>> +++ b/include/linux/fscrypt.h
>> @@ -951,10 +951,29 @@ static inline int fscrypt_prepare_rename(struct inode *old_dir,
>> static inline void fscrypt_prepare_dentry(struct dentry *dentry,
>> bool is_nokey_name)
>> {
>> + /*
>> + * This code tries to only take ->d_lock when necessary to write
>> + * to ->d_flags. We shouldn't be peeking on d_flags for
>> + * DCACHE_OP_REVALIDATE unlocked, but in the unlikely case
>> + * there is a race, the worst it can happen is that we fail to
>> + * unset DCACHE_OP_REVALIDATE and pay the cost of an extra
>> + * d_revalidate.
>> + */
>> if (is_nokey_name) {
>> spin_lock(&dentry->d_lock);
>> 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.
>> + */
>> + spin_lock(&dentry->d_lock);
>> + dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
>> + spin_unlock(&dentry->d_lock);
>> }
>> }
>
> Does this all get optimized out when !CONFIG_FS_ENCRYPTION?
>
> As-is, I don't think the d_revalidate part will be optimized out.
>
it seems to get optimized out:
This is ext4_lookup built with CONFIG_FS_ENCRYPTION=n
ffffffff814ca3e0 <ext4_lookup>:
ffffffff814ca3e0: e8 5b b5 c3 ff call ffffffff81105940 <__fentry__>
ffffffff814ca3e5: 41 54 push %r12
ffffffff814ca3e7: 55 push %rbp
ffffffff814ca3e8: 53 push %rbx
ffffffff814ca3e9: 48 83 ec 58 sub $0x58,%rsp
ffffffff814ca3ed: 8b 56 24 mov 0x24(%rsi),%edx
ffffffff814ca3f0: 65 48 8b 04 25 28 00 mov %gs:0x28,%rax
ffffffff814ca3f7: 00 00
ffffffff814ca3f9: 48 89 44 24 50 mov %rax,0x50(%rsp)
ffffffff814ca3fe: 31 c0 xor %eax,%eax
ffffffff814ca400: 48 c7 c0 dc ff ff ff mov $0xffffffffffffffdc,%rax
ffffffff814ca407: 81 fa ff 00 00 00 cmp $0xff,%edx
ffffffff814ca40d: 76 21 jbe ffffffff814ca430 <ext4_lookup+0x50>
ffffffff814ca40f: 48 8b 4c 24 50 mov 0x50(%rsp),%rcx
ffffffff814ca414: 65 48 33 0c 25 28 00 xor %gs:0x28,%rcx
ffffffff814ca41b: 00 00
ffffffff814ca41d: 0f 85 cd 01 00 00 jne ffffffff814ca5f0 <ext4_lookup+0x210> <- (__stack_chk_fail)
ffffffff814ca423: 48 83 c4 58 add $0x58,%rsp
ffffffff814ca427: 5b pop %rbx
ffffffff814ca428: 5d pop %rbp
ffffffff814ca429: 41 5c pop %r12
ffffffff814ca42b: e9 70 21 8b 00 jmp ffffffff81d7c5a0 <__x86_return_thunk>
ffffffff814ca430: 48 89 f3 mov %rsi,%rbx
ffffffff814ca433: 89 54 24 20 mov %edx,0x20(%rsp)
ffffffff814ca437: 48 8d 76 20 lea 0x20(%rsi),%rsi
ffffffff814ca43b: 48 8b 43 28 mov 0x28(%rbx),%rax
ffffffff814ca43f: 48 8d 54 24 10 lea 0x10(%rsp),%rdx
ffffffff814ca444: 48 89 fd mov %rdi,%rbp
ffffffff814ca447: 48 89 74 24 10 mov %rsi,0x10(%rsp)
ffffffff814ca44c: 48 89 44 24 18 mov %rax,0x18(%rsp)
ffffffff814ca451: e8 ca f0 ff ff call ffffffff814c9520 <ext4_fname_setup_ci_filename>
[..]
I had also confirmed previously that fscrypt_lookup_prepare and
fscrypt_prepare_dentry gets correctly inlined into
ext4_fname_prepare_lookup.
> You may need to create a !CONFIG_FS_ENCRYPTION stub explicitly.
But, in spite of gcc doing the right thing now, fscrypt_prepare_dentry
might grow in the future. So, if you don't mind, I will still add the
stub explicitly, as you suggested.
thanks,
--
Gabriel Krisman Bertazi
