linux-ext4 - Re: [PATCH v6 04/10] fscrypt: Drop d_revalidate once the key is added

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240215003145.GK1638@sol.localdomain>
Date: Wed, 14 Feb 2024 16:31:45 -0800
From: Eric Biggers <ebiggers@...nel.org>
To: Gabriel Krisman Bertazi <krisman@...e.de>
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, brauner@...nel.org
Subject: Re: [PATCH v6 04/10] fscrypt: Drop d_revalidate once the key is added

On Wed, Feb 14, 2024 at 04:16:31PM -0800, Eric Biggers wrote:
> On Mon, Feb 12, 2024 at 09:13:15PM -0500, Gabriel Krisman Bertazi wrote:
> > From fscrypt perspective, once the key is available, the dentry will
> > remain valid until evicted for other reasons, since keyed dentries don't
> > require revalidation and, if the key is removed, the dentry is
> > forcefully evicted.  Therefore, we don't need to keep revalidating them
> > repeatedly.
> > 
> > Obviously, we can only do this if fscrypt is the only thing requiring
> > revalidation for a dentry.  For this reason, we only disable
> > d_revalidate if the .d_revalidate hook is fscrypt_d_revalidate itself.
> > 
> > It is safe to do it here because when moving the dentry to the
> > plain-text version, we are holding the d_lock.  We might race with a
> > concurrent RCU lookup but this is harmless because, at worst, we will
> > get an extra d_revalidate on the keyed dentry, which is will find the
> > dentry is valid.
> > 
> > Finally, now that we do more than just clear the DCACHE_NOKEY_NAME in
> > fscrypt_handle_d_move, skip it entirely for plaintext dentries, to avoid
> > extra costs.
> > 
> > Signed-off-by: Gabriel Krisman Bertazi <krisman@...e.de>
> 
> I think this explanation misses an important point, which is that it's only
> *directories* where a no-key dentry can become the regular dentry.  The VFS does
> the move because it only allows one dentry to exist per directory.
> 
> For nondirectories, the dentries don't get reused and this patch is irrelevant.
> 
> (Of course, there's no point in making fscrypt_handle_d_move() check whether the
> dentry is a directory, since checking DCACHE_NOKEY_NAME is sufficient.)
> 
> The diff itself looks good -- thanks.
> 

Also, do I understand correctly that this patch is a performance optimization,
not preventing a performance regression?  The similar patch that precedes this
one, "fscrypt: Drop d_revalidate for valid dentries during lookup", is about
preventing a performance regression on dentries that aren't no-key.  This patch
looks deceptively similar, but it only affects no-key directory dentries, which
we were already doing the fscrypt_d_revalidate for, even after the move to the
plaintext name.  It's probably still a worthwhile optimization to stop doing the
fscrypt_d_revalidate when a directory dentry gets moved like that.  But I want
to make sure I'm correctly understanding each patch.

- Eric