[PATCH 1/5] file: depend on the reloaded pointer in __fget_files_rcu()

[Christian Brauner <brauner@kernel.org>]

__fget_files_rcu() loads the file pointer, takes a reference with
file_ref_get(), reloads the pointer and compares the two. If they match
it returns the pointer of the first load. The caller will read f_mode,
f_op and everything else through.

Becuase of SLAB_TYPESAFE_BY_RCU the file can be closed, freed and
reallocated between the first load and the increment. If another thread
of the same process reopens the same descriptor the reload can see the
same pointer again and the recheck passes.

While that's fine because the descriptor does point to that file now the
subsequent loads are only ordered after the increment.

The increment synchronizes with the store of file_ref_init() and
init_file(). But that happens before do_dentry_open() sets f_op and most
of the other fields.

They are published by the rcu_assign_pointer() in fd_install() meaning
the reload of the pointer.

But there's no dependency between the reload and later loads. So on
arm64 or power the caller can read the f_mode and the f_op of the
previous lifetime of the file.

So we do need to compare against a copy the compiler can't see through
and then use the reloaded pointer from there on just like we already do
in __get_file_rcu().

Nothing measurable changes because of this change.

Fixes: 253ca8678d30 ("Improve __fget_files_rcu() code generation (and thus __fget_light())")
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/file.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/fs/file.c b/fs/file.c
index 4cd40dfa6129..052da9409874 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -1168,6 +1168,8 @@ static inline struct file *__fget_files_rcu(struct files_struct *files,
 {
 	for (;;) {
 		struct file *file;
+		struct file *file_reloaded;
+		struct file *file_reloaded_cmp;
 		struct fdtable *fdt = rcu_dereference_raw(files->fdt);
 		struct file __rcu **fdentry;
 		unsigned long nospec_mask;
@@ -1216,11 +1218,32 @@ static inline struct file *__fget_files_rcu(struct files_struct *files,
 		 *
 		 * If so, we need to put our ref and try again.
 		 */
-		if (unlikely(file != rcu_dereference_raw(*fdentry)) ||
+		file_reloaded = rcu_dereference_raw(*fdentry);
+
+		/*
+		 * Every load needs to depend on the reload. If the
+		 * file was freed and reallocated into this slot in the
+		 * meantime, what open() wrote to it was published by the
+		 * rcu_assign_pointer() in fd_install(), which is the
+		 * store the reload saw.
+		 *
+		 * The increment above only orders against file_ref_init()
+		 * and init_file(). That happens before do_dentry_open()
+		 * sets f_op et al. So every load needs to depend on the
+		 * reload otherwise we can see e.g., stale f_op on e.g.,
+		 * arm64.
+		 *
+		 * So compare against a copy the compiler can't see through
+		 * and use the reloaded pointer from here on.
+		 */
+		file_reloaded_cmp = file_reloaded;
+		OPTIMIZER_HIDE_VAR(file_reloaded_cmp);
+		if (unlikely(file != file_reloaded_cmp) ||
 		    unlikely(rcu_dereference_raw(files->fdt) != fdt)) {
 			fput(file);
 			continue;
 		}
+		file = file_reloaded;
 
 		/*
 		 * This isn't the file we're looking for or we're not

-- 
2.53.0