Received-SPF: pass (google.com: domain of linux-ext4+bounces-1301-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: Gabriel Krisman Bertazi <krisman@suse.de>
To: Eric Biggers <ebiggers@kernel.org>
Cc: viro@zeniv.linux.org.uk,  jaegeuk@kernel.org,  tytso@mit.edu,
  amir73il@gmail.com,  linux-ext4@vger.kernel.org,
  linux-f2fs-devel@lists.sourceforge.net,  linux-fsdevel@vger.kernel.org,
  brauner@kernel.org
Subject: Re: [PATCH v6 03/10] fscrypt: Drop d_revalidate for valid dentries
 during lookup
In-Reply-To: <20240214235904.GH1638@sol.localdomain> (Eric Biggers's message
	of "Wed, 14 Feb 2024 15:59:04 -0800")
Organization: SUSE
References: <20240213021321.1804-1-krisman@suse.de>
	<20240213021321.1804-4-krisman@suse.de>
	<20240214235904.GH1638@sol.localdomain>
Date: Tue, 20 Feb 2024 18:03:13 -0500
Message-ID: <87o7caagcu.fsf@mailhost.krisman.be>
User-Agent: Gnus/5.13 (Gnus v5.13)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

Eric Biggers <ebiggers@kernel.org> 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