fs-verity: use smp_load_acquire() for ->i_verity_info (f3db0bed) · Commits · 戴 / test

Normally smp_store_release() or cmpxchg_release() is paired with smp_load_acquire(). Sometimes smp_load_acquire() can be replaced with the more lightweight READ_ONCE(). However, for this to be safe, all the published memory must only be accessed in a way that involves the pointer itself. This may not be the case if allocating the object also involves initializing a static or global variable, for example. fsverity_info::tree_params.hash_alg->tfm is a crypto_ahash object that's internal to and is allocated by the crypto subsystem. So by using READ_ONCE() for ->i_verity_info, we're relying on internal implementation details of the crypto subsystem. Remove this fragile assumption by using smp_load_acquire() instead. Also fix the cmpxchg logic to correctly execute an ACQUIRE barrier when losing the cmpxchg race, since cmpxchg doesn't guarantee a memory barrier on failure. (Note: I haven't seen any real-world problems here. This change is just fixing the code to be guaranteed correct and less fragile.) Fixes: ("fs-verity: add the hook for file ->open()") Link: https://lore.kernel.org/r/20200721225920.114347-6-ebiggers@kernel.org Signed-off-by:

Eric Biggers <ebiggers@google.com>

fs/verity/open.c

+12 −3

Original line number	Diff line number	Diff line
		@@ -221,11 +221,20 @@ out:
		void fsverity_set_info(struct inode inode, struct fsverity_info vi)
		{
		/*
		* Multiple processes may race to set ->i_verity_info, so use cmpxchg.
		* This pairs with the READ_ONCE() in fsverity_get_info().
		* Multiple tasks may race to set ->i_verity_info, so use
		* cmpxchg_release(). This pairs with the smp_load_acquire() in
		* fsverity_get_info(). I.e., here we publish ->i_verity_info with a
		* RELEASE barrier so that other tasks can ACQUIRE it.
		*/
		if (cmpxchg(&inode->i_verity_info, NULL, vi) != NULL)
		if (cmpxchg_release(&inode->i_verity_info, NULL, vi) != NULL) {
		/* Lost the race, so free the fsverity_info we allocated. */
		fsverity_free_info(vi);
		/*
		* Afterwards, the caller may access ->i_verity_info directly,
		* so make sure to ACQUIRE the winning fsverity_info.
		*/
		(void)fsverity_get_info(inode);
		}
		}

		void fsverity_free_info(struct fsverity_info *vi)

include/linux/fsverity.h

+7 −2

Original line number	Diff line number	Diff line
		@@ -115,8 +115,13 @@ struct fsverity_operations {

		static inline struct fsverity_info fsverity_get_info(const struct inode inode)
		{
		/* pairs with the cmpxchg() in fsverity_set_info() */
		return READ_ONCE(inode->i_verity_info);
		/*
		* Pairs with the cmpxchg_release() in fsverity_set_info().
		* I.e., another task may publish ->i_verity_info concurrently,
		* executing a RELEASE barrier. We need to use smp_load_acquire() here
		* to safely ACQUIRE the memory the other task published.
		*/
		return smp_load_acquire(&inode->i_verity_info);
		}

		/* enable.c */

Admin message