fs-verity: use smp_load_acquire() for ->i_verity_info

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: fd2d1acfcadf ("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>

diff --git a/fs/verity/open.c b/fs/verity/open.c
index d007db0c9304d1f7461080ce6332c7022158efd1..bfe0280c14e49a7e25408f08bb66dd5ca1641251 100644 (file)
--- a/fs/verity/open.c
+++ b/fs/verity/open.c
@@ -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)

diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h
index 78201a6d35f66d8b077f0d9faab6bc8be1315750..c1144a4503920933cf13908c43d6882e8cf310e9 100644 (file)
--- a/include/linux/fsverity.h
+++ b/include/linux/fsverity.h
@@ -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 */