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@xxxxxxxxxx> 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
|