[PATCH 2/5] file_ref: use acquire ordering in file_ref_get()

[Christian Brauner <brauner@kernel.org>]

file_ref_get() is fully ordered because the fget-rcu pattern needs the
loads after it ordered after the increment. This is because
__fget_files_rcu() reloads the descriptor slot and compares and returns
the file only if the reload still matches.

If the reload could be satisfied before the increment, the lookup would
pass its recheck on a file that was closed, freed and reallocated in
between and hand the caller the wrong file.

This is satisfied by acquire ordering. The load of the pointer the
reference is taken through is ordered before the increment by the
address dependency of &file->f_ref on it.

The release part isn't needed. Someone getting a reference never has
stores before it that the put side has to see.

The comment in __fget_files_rcu() references that fact. But it's no
longer true since we're not using atomic_long_inc_not_zero() anymore.
file_ref has one so we can use it.

So I had a version of this within in the form of a relaxed increment
with an smp_load_acquire() on the reload. Linus rightly pointed out that
this doesn't work in [1].

Let's use atomic_long_add_negative_acquire(). That's a nop change for x86.
On arm64 and power this is an improvement.

Link: https://lore.kernel.org/all/CAHk-=wj7=Ynmk9+Fm860NqHu5q119AiN4YNXNJPt=6Q=Y=w3HA@mail.gmail.com/ [1]
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/file.c                | 17 ++++++++---------
 include/linux/file_ref.h | 10 +++++++---
 2 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 052da9409874..fc4b81328ae2 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -1096,13 +1096,13 @@ static struct file *__get_file_rcu(struct file __rcu **f)
 	OPTIMIZER_HIDE_VAR(file_reloaded_cmp);
 
 	/*
-	 * file_ref_get() above provided a full memory barrier when we
-	 * acquired a reference.
+	 * file_ref_get() above is an acquire, so the reload of the
+	 * pointer is ordered after the increment.
 	 *
-	 * This is paired with the write barrier from assigning to the
-	 * __rcu protected file pointer so that if that pointer still
-	 * matches the current file, we know we have successfully
-	 * acquired a reference to the right file.
+	 * This is paired with the release of the rcu_assign_pointer()
+	 * that published the __rcu protected file pointer so that if
+	 * that pointer still matches the current file, we know we have
+	 * successfully acquired a reference to the right file.
 	 *
 	 * If the pointers don't match the file has been reallocated by
 	 * SLAB_TYPESAFE_BY_RCU.
@@ -1197,9 +1197,8 @@ static inline struct file *__fget_files_rcu(struct files_struct *files,
 		 * We need to confirm it by incrementing the refcount
 		 * and then check the lookup again.
 		 *
-		 * file_ref_get() gives us a full memory barrier. We
-		 * only really need an 'acquire' one to protect the
-		 * loads below, but we don't have that.
+		 * file_ref_get() is an acquire, which is what orders
+		 * the loads below after the increment.
 		 */
 		if (unlikely(!file_ref_get(&file->f_ref)))
 			continue;
diff --git a/include/linux/file_ref.h b/include/linux/file_ref.h
index 31551e4cb8f3..4cffb24257e2 100644
--- a/include/linux/file_ref.h
+++ b/include/linux/file_ref.h
@@ -69,7 +69,11 @@ bool __file_ref_put(file_ref_t *ref, unsigned long cnt);
  *
  * Similar to atomic_inc_not_zero() but saturates at FILE_REF_MAXREF.
  *
- * Provides full memory ordering.
+ * Provides acquire memory ordering: loads after a successful get are
+ * ordered after the increment. That is what the pointer recheck of the
+ * fget-rcu pattern needs, the load of the pointer the reference is
+ * taken through is ordered before the increment by its address
+ * dependency.
  *
  * Return: False if the attempt to acquire a reference failed. This happens
  *         when the last reference has been put already. True if a reference
@@ -78,7 +82,7 @@ bool __file_ref_put(file_ref_t *ref, unsigned long cnt);
 static __always_inline __must_check bool file_ref_get(file_ref_t *ref)
 {
 	/*
-	 * Unconditionally increase the reference count with full
+	 * Unconditionally increase the reference count with acquire
 	 * ordering. The saturation and dead zones provide enough
 	 * tolerance for this.
 	 *
@@ -105,7 +109,7 @@ static __always_inline __must_check bool file_ref_get(file_ref_t *ref)
 	 * FILE_REF_ONEREF. It would need 2305843009213693952/2^61
 	 * file_ref_get()s to resurrect such a dead file.
 	 */
-	return !atomic_long_add_negative(1, &ref->refcnt);
+	return !atomic_long_add_negative_acquire(1, &ref->refcnt);
 }
 
 /**

-- 
2.53.0