Re: [RFC PATCH 01/17] fscrypt: factor accessing inode->i_crypt_info

[Eric Biggers <ebiggers@kernel.org>]

On Sun, Jan 01, 2023 at 12:06:05AM -0500, Sweet Tea Dorminy wrote:
> Currently, inode->i_crypt_info is accessed directly in many places;
> the initial setting occurs in one place, via cmpxchg_release, and
> the initial access is abstracted into fscrypt_get_info() which uses
> smp_load_acquire(), but there are many direct accesses. While many of
> them follow calls to fscrypt_get_info() on the same thread, verifying
> this is not always trivial.
> 
> For instance, fscrypt_crypt_block() does not obviously follow a call to
> fscrypt_get_info() on the same cpu; if some other mechanism does not
> ensure a memory barrier, it is conceivable that a filesystem could call
> fscrypt_crypt_block() on a cpu which had an old (NULL) i_crypt_info
> cached. Even if the cpu does READ_ONCE(i_crypt_info), I believe it's
> theoretically possible for it to see the old NULL value, since this
> could be happening on a cpu which did not do the smp_load_acquire().  (I
> may be misunderstanding, but memory-barriers.txt says that only the cpus
> involved in an acquire/release chain are guaranteed to see the correct
> order of operations, which seems to imply that a cpu which does not do
> an acquire may be able to see a memory value from before the release.)
> 
> For safety, then, and so each site doesn't need to be individually
> evaluated, this change factors all accesses of i_crypt_info to go
> through fscrypt_get_info(), ensuring every access uses acquire and is
> thus paired against an appropriate release.
> 
> (The same treatment is not necessary for setting i_crypt_info; the
> only unprotected setting is during inode cleanup, which is inevitably
> followed by freeing the inode; there are no uses past the unprotected
> setting possible.)
> 
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>

This patch is not necessary.  The rules for accessing ->i_crypt_info are
actually pretty simple: when it's unknown whether ->i_crypt_info has been set,
then it's necessary to use fscrypt_get_info() and check whether the resulting
pointer is NULL or not (or use fscrypt_has_encryption_key() which does both).
That's because another thread could set it concurrently.

In contrast, when it *is* known that ->i_crypt_info has been set, then that can
only be because fscrypt_has_encryption_key() was already executed on the same
thread, or because an operation that ensured the key is set up already happened.
For example, when doing I/O to a file, it's guaranteed that the file has been
opened.  In either case, direct access is fine.

- Eric