[PATCH 0/5] file: fix and document ordering for lockless fd lookup wrt to file_ref

[PATCH 0/5] file: fix and document ordering for lockless fd lookup wrt to file_ref

[Christian Brauner]

I went through include/linux/file_ref.h and its lockless users with a
TLA+ model and a set of LKMM litmus tests:

__fget_files_rcu(), __get_file_rcu(), fput() and file_ref_put_close()
against close(), open() and the SLAB_TYPESAFE_BY_RCU recycling of the
file. The protocol itself is fine but there are a few minor things:

* __fget_files_rcu() lost the dependency on its reload in
  253ca8678d30 making it possible to see f_mode and f_op from an earlier
  lifetime of a file on arm64 and power.

* file_ref_get() only needs acquire ordering. The reload has to be
  ordered after the increment. This is all we need.

* Add some missing documentation why we need full ordering during put.

This also adds litmus tests for this stuff. Here are the numbers after
this series:
                       instructions        cycles
    fcntl(F_GETFL)      457 -> 463    217.6 -> 218.1
    fstat()            1052 -> 1058   455.4 -> 454.1
    poll(), 16 fds     2394 -> 2458   828.7 -> 833.4
    pipe write+read    2261 -> 2271   925.1 -> 913.0
    unix socketpair    3269 -> 3277  1202.3 -> 1184.8
    io_uring_enter()   1036 -> 1038   284.6 -> 286.5
      registered file   942 -> 942    262.6 -> 263.2
    open()+close()     4277 -> 4277  1365.7 -> 1334.7
    dup()+close()       869 -> 873    525.1 -> 520.5

It's all within noise in terms of performance.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
Christian Brauner (5):
      file: depend on the reloaded pointer in __fget_files_rcu()
      file_ref: use acquire ordering in file_ref_get()
      file_ref: document why file_ref_put() stays fully ordered
      Documentation/litmus-tests: add the file reference count tests
      Documentation: update the lock-free file lookup in files.rst

 Documentation/filesystems/files.rst                | 48 +++++++++-----
 Documentation/litmus-tests/README                  | 57 ++++++++++++++++
 .../fs/file-ref-get-acquire-recheck.litmus         | 66 +++++++++++++++++++
 .../fs/file-ref-get-relaxed-recheck.litmus         | 50 ++++++++++++++
 .../fs/file-ref-put-full-handoff.litmus            | 76 ++++++++++++++++++++++
 .../fs/file-ref-put-release-handoff.litmus         | 66 +++++++++++++++++++
 .../fs/file-ref-reload-dep-field.litmus            | 74 +++++++++++++++++++++
 .../fs/file-ref-reload-nodep-field.litmus          | 59 +++++++++++++++++
 fs/file.c                                          | 42 +++++++++---
 include/linux/file_ref.h                           | 21 +++++-
 10 files changed, 528 insertions(+), 31 deletions(-)
---
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
change-id: 20261105-work-file_ref-ordering-00ddcd27bf2d

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

[Christian Brauner]

__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

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

[Christian Brauner]

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

[PATCH 3/5] file_ref: document why file_ref_put() stays fully ordered

[Christian Brauner]

The comment above file_ref_put() describes release ordering on the way
in and acquire ordering on success. file_ref_put() uses the fully
ordered atomic_long_dec_return() instead. Document why we need it.

Files are SLAB_TYPESAFE_BY_RCU. When the last reference goes
synchronously, in __fput_sync() and on the fallback path of
fput_close_sync(), the file is freed right after the decrement and
another CPU can get it from the slab and reinitialize the counter at
once. The lookup that increments that recycled counter relies on seeing
the stores the closer made before its put. A release orders those stores
before the decrement, not before the slab handoff that follows it. The
full barrier does. The deferred fput() path has the cmpxchg() in
task_work_add() between the decrement and the free, the synchronous
paths have nothing else.

No functional changes.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 include/linux/file_ref.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/file_ref.h b/include/linux/file_ref.h
index 4cffb24257e2..e5fc45714fe2 100644
--- a/include/linux/file_ref.h
+++ b/include/linux/file_ref.h
@@ -133,6 +133,17 @@ static __always_inline void file_ref_inc(file_ref_t *ref)
  * are done before, and provides an acquire ordering on success such
  * that free() must come after.
  *
+ * The decrement is fully ordered though and it has to stay that way.
+ * When the last reference goes synchronously, in __fput_sync() and on
+ * the fallback path of fput_close_sync(), the full barrier is what
+ * orders the caller's earlier stores, in close(2) the cleared slot in
+ * the descriptor table, before the slab handoff that lets another CPU
+ * reallocate the file. A lookup that increments the recycled counter
+ * then sees the cleared slot and fails its recheck. The deferred
+ * fput() has the cmpxchg() of task_work_add() for that, the
+ * synchronous paths have nothing else. So this is not a candidate for
+ * atomic_long_dec_return_release().
+ *
  * Return: True if this was the last reference with no future references
  *         possible. This signals the caller that it can safely release
  *         the object which is protected by the reference counter.

-- 
2.53.0

[PATCH 4/5] Documentation/litmus-tests: add the file reference count tests

[Christian Brauner]

A struct file is SLAB_TYPESAFE_BY_RCU and __fget_files_rcu() does a
lockless lookup against that: it loads the descriptor slot, takes a
reference with file_ref_get(), reloads the slot and compares. Between
the first load and the increment the file can be closed, freed and
handed out again by open(), on the same CPU or on another one through
the slab freelist. The three patches before this one rest on three
orderings in that dance:

(1) acquire ordering on file_ref_get: The reload has to be ordered
    after the increment or the recheck can read the old slot after the
    increment hit the recycled file.

(2) reload dependency: What the caller reads from the file has to depend
    on the reload, not on the first load otherwise a thread that closes
    and reopens the same descriptor and gets the same file back lets a
    concurrent lookup read the old one's f_op.

(3) full ordering on file_ref_put(): in the synchronous free paths
    nothing else orders the cleared store before the freelist store that
    hands out the object to another CPU.

Add litmus tests so we can check that against the kernel memory model.
Each ordering gets a test of the code as it is after this series.

The generic facts these rest on are in tools/memory-model already and
aren't repeated: MP+onceassign+derefonce for the dependency that
orders a load after the release store its pointer came from, and
Z6.0+pooncerelease+poacquirerelease+fencembonceonce for why a
release-acquire chain doesn't make an earlier store visible to a third
CPU without a full barrier.

What's specific to file_ref is the counter being set up again on a
recycled object, the recheck, and the handoff through the freelist, and
that's what the tests are about.

Run them with herd7 against tools/memory-model:

    cd tools/memory-model
    scripts/checklitmus.sh ../../Documentation/litmus-tests/fs/<test>

A "Never" there holds on every architecture, since each one's atomics,
barriers and RCU have to follow the memory model.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 Documentation/litmus-tests/README                  | 57 ++++++++++++++++
 .../fs/file-ref-get-acquire-recheck.litmus         | 66 +++++++++++++++++++
 .../fs/file-ref-get-relaxed-recheck.litmus         | 50 ++++++++++++++
 .../fs/file-ref-put-full-handoff.litmus            | 76 ++++++++++++++++++++++
 .../fs/file-ref-put-release-handoff.litmus         | 66 +++++++++++++++++++
 .../fs/file-ref-reload-dep-field.litmus            | 74 +++++++++++++++++++++
 .../fs/file-ref-reload-nodep-field.litmus          | 59 +++++++++++++++++
 7 files changed, 448 insertions(+)

diff --git a/Documentation/litmus-tests/README b/Documentation/litmus-tests/README
index 6c666f3422ea..ce3a089efdac 100644
--- a/Documentation/litmus-tests/README
+++ b/Documentation/litmus-tests/README
@@ -38,6 +38,63 @@ cmpxchg-fail-unordered-2.litmus
     acquire operation.
 
 
+fs (/fs directory)
+------------------
+
+A struct file is SLAB_TYPESAFE_BY_RCU.  __fget_files_rcu() loads the
+descriptor slot, takes a reference with file_ref_get(), reloads the
+slot and compares, and between the first load and the increment the
+file can be closed, freed and handed out again.  The tests encode the
+counter as an atomic_t holding references minus one (ONEREF 0, NOREF
+-1, DEAD -4), file_ref_get() as atomic_fetch_add() with the ordering
+under test, file_ref_put() as atomic_fetch_sub() followed by the
+cmpxchg to DEAD of __file_ref_put(), init_file() as a plain
+atomic_set() and fd_install() as a release store.  The slot is an int
+where only its identity matters and a pointer where a later load has
+to depend on it.  The counter is a separate variable, so the address
+dependency of the increment on the first load is not in the model;
+that only makes the model weaker than the code, a "Never" here holds
+for the code.
+
+file-ref-get-acquire-recheck.litmus
+    Demonstrates that an acquire-ordered file_ref_get() is enough for
+    the recheck of __fget_files_rcu(): the full ordering of the
+    decrement orders the cleared slot before the counter is set up
+    again and the acquire orders the reload after the increment, so a
+    lookup that increments a recycled file cannot pass the recheck with
+    the old slot.
+
+file-ref-get-relaxed-recheck.litmus
+    The same with a relaxed increment; the reload can be satisfied
+    before the increment and the lookup returns a file the descriptor
+    never named.
+
+file-ref-reload-dep-field.litmus
+    Demonstrates that reading the file's fields through the reloaded
+    pointer, as __get_file_rcu() does, orders them after the store of
+    fd_install() that the reload saw: a thread that closes and reopens
+    the same descriptor and gets the same file back cannot make a
+    concurrent lookup read the old incarnation's f_op.
+
+file-ref-reload-nodep-field.litmus
+    The same with the fields read through the pointer of the first
+    load; the field read is ordered after the increment but not after
+    the reload, and a fully ordered increment does not change that.
+
+file-ref-put-full-handoff.litmus
+    Demonstrates why file_ref_put() keeps its full ordering: when the
+    last put frees the file synchronously and another CPU takes it from
+    the slab freelist, the fully ordered decrement orders the cleared
+    slot before the free, so a lookup on a third CPU that increments
+    the recycled counter cannot pass its recheck with the old slot.
+
+file-ref-put-release-handoff.litmus
+    The same with a release-ordered decrement; it orders the cleared
+    slot before the decrement and not before the free, and the lookup
+    can keep a reference to a recycled file for a descriptor that never
+    named it.
+
+
 locking (/locking directory)
 ----------------------------
 
diff --git a/Documentation/litmus-tests/fs/file-ref-get-acquire-recheck.litmus b/Documentation/litmus-tests/fs/file-ref-get-acquire-recheck.litmus
new file mode 100644
index 000000000000..576b5ba2d2e8
--- /dev/null
+++ b/Documentation/litmus-tests/fs/file-ref-get-acquire-recheck.litmus
@@ -0,0 +1,66 @@
+C file-ref-get-acquire-recheck
+
+(*
+ * Result: Never
+ *
+ * __fget_files_rcu() loads the descriptor slot, takes a reference with
+ * file_ref_get() and reloads the slot to check that it still holds the
+ * same file.  Files are SLAB_TYPESAFE_BY_RCU, so between the first load
+ * and the increment the file can be closed, freed and handed out again
+ * by open(), and the increment then lands on the recycled file.  The
+ * recheck catches that only if the reload is ordered after the
+ * increment.
+ *
+ * P0 is the owner: close() clears the slot in file_close_fd_locked()
+ * with a plain store of NULL, fput() drops the last reference with the
+ * fully ordered decrement of file_ref_put() and the cmpxchg to DEAD of
+ * __file_ref_put(), __fput() frees the file, and open() gets the same
+ * object back: init_file() sets ONEREF with a plain store and
+ * fd_install() publishes it in another slot with a release store.  P1
+ * is the lookup with an acquire-ordered file_ref_get(), i.e.
+ * atomic_long_add_negative_acquire().
+ *
+ * The full ordering of the decrement orders the cleared slot before the
+ * counter's re-initialisation and the acquire orders the reload after
+ * the increment.  So once the increment has hit the recycled counter
+ * (1:r1=0 after the successful cmpxchg, 0:r1=-1) the reload sees the
+ * cleared slot and the lookup retries.
+ *)
+
+{
+	atomic_t cnt = ATOMIC_INIT(0);
+	slot=1;
+	slot2=0;
+	0:r1=-9;
+}
+
+P0(int *slot, int *slot2, atomic_t *cnt)
+{
+	int r0;
+	int r1;
+
+	WRITE_ONCE(*slot, 0);
+	r0 = atomic_fetch_sub(1, cnt);
+	if (r0 == 0) {
+		r1 = atomic_cmpxchg_release(cnt, -1, -4);
+		if (r1 == -1) {
+			atomic_set(cnt, 0);
+			smp_store_release(slot2, 1);
+		}
+	}
+}
+
+P1(int *slot, atomic_t *cnt)
+{
+	int r0;
+	int r1;
+	int r2;
+
+	rcu_read_lock();
+	r0 = READ_ONCE(*slot);
+	r1 = atomic_fetch_add_acquire(1, cnt);
+	r2 = READ_ONCE(*slot);
+	rcu_read_unlock();
+}
+
+exists (0:r1=-1 /\ 1:r0=1 /\ 1:r1=0 /\ 1:r2=1)
diff --git a/Documentation/litmus-tests/fs/file-ref-get-relaxed-recheck.litmus b/Documentation/litmus-tests/fs/file-ref-get-relaxed-recheck.litmus
new file mode 100644
index 000000000000..6f095eeafdce
--- /dev/null
+++ b/Documentation/litmus-tests/fs/file-ref-get-relaxed-recheck.litmus
@@ -0,0 +1,50 @@
+C file-ref-get-relaxed-recheck
+
+(*
+ * Result: Sometimes
+ *
+ * file-ref-get-acquire-recheck.litmus with a relaxed increment.  Nothing
+ * orders the reload after the increment anymore, so the reload can be
+ * satisfied before the increment hits the recycled counter: the lookup
+ * holds a reference to the recycled file and its recheck still saw the
+ * old slot, so it returns a file the descriptor never named.  Acquire
+ * is the minimum for file_ref_get().
+ *)
+
+{
+	atomic_t cnt = ATOMIC_INIT(0);
+	slot=1;
+	slot2=0;
+	0:r1=-9;
+}
+
+P0(int *slot, int *slot2, atomic_t *cnt)
+{
+	int r0;
+	int r1;
+
+	WRITE_ONCE(*slot, 0);
+	r0 = atomic_fetch_sub(1, cnt);
+	if (r0 == 0) {
+		r1 = atomic_cmpxchg_release(cnt, -1, -4);
+		if (r1 == -1) {
+			atomic_set(cnt, 0);
+			smp_store_release(slot2, 1);
+		}
+	}
+}
+
+P1(int *slot, atomic_t *cnt)
+{
+	int r0;
+	int r1;
+	int r2;
+
+	rcu_read_lock();
+	r0 = READ_ONCE(*slot);
+	r1 = atomic_fetch_add_relaxed(1, cnt);
+	r2 = READ_ONCE(*slot);
+	rcu_read_unlock();
+}
+
+exists (0:r1=-1 /\ 1:r0=1 /\ 1:r1=0 /\ 1:r2=1)
diff --git a/Documentation/litmus-tests/fs/file-ref-put-full-handoff.litmus b/Documentation/litmus-tests/fs/file-ref-put-full-handoff.litmus
new file mode 100644
index 000000000000..158bcbc82779
--- /dev/null
+++ b/Documentation/litmus-tests/fs/file-ref-put-full-handoff.litmus
@@ -0,0 +1,76 @@
+C file-ref-put-full-handoff
+
+(*
+ * Result: Never
+ *
+ * Why file_ref_put() keeps its full ordering.  When the last put frees
+ * the file synchronously, in __fput_sync() or on the fallback path of
+ * fput_close_sync(), the object goes to another CPU through the slab
+ * freelist, that CPU recycles it, sets ONEREF and publishes it in a
+ * slot of its own, and a lookup on a third CPU can increment the
+ * recycled counter.  Its recheck must then see the cleared slot, which
+ * needs the slot clear ordered before the free.
+ *
+ * P0 is the owner: close() clears the slot, the fully ordered decrement
+ * of file_ref_put() reaches NOREF, the cmpxchg of __file_ref_put()
+ * marks the file DEAD, __fput() frees it and the object lands on the
+ * freelist, a relaxed RMW in SLUB and a plain store here.  P2 is open()
+ * on another CPU: it takes the object from the freelist, a load that
+ * the stores of init_file() depend on by control, sets ONEREF and
+ * publishes the file with fd_install().  P1 is __fget_files_rcu() with
+ * an acquire-ordered file_ref_get().
+ *
+ * The full barrier of the decrement orders the cleared slot before the
+ * freelist store.  P2's control dependency orders its stores after the
+ * freelist load, the acquire on P1 orders the reload after the
+ * increment that read P2's ONEREF, so the reload sees the cleared slot.
+ *)
+
+{
+	atomic_t cnt = ATOMIC_INIT(0);
+	slot=1;
+	slot2=0;
+	fl=0;
+	0:r1=-9;
+}
+
+P0(int *slot, int *fl, atomic_t *cnt)
+{
+	int r0;
+	int r1;
+
+	WRITE_ONCE(*slot, 0);
+	r0 = atomic_fetch_sub(1, cnt);
+	if (r0 == 0) {
+		r1 = atomic_cmpxchg_release(cnt, -1, -4);
+		if (r1 == -1) {
+			WRITE_ONCE(*fl, 1);
+		}
+	}
+}
+
+P1(int *slot, atomic_t *cnt)
+{
+	int r0;
+	int r1;
+	int r2;
+
+	rcu_read_lock();
+	r0 = READ_ONCE(*slot);
+	r1 = atomic_fetch_add_acquire(1, cnt);
+	r2 = READ_ONCE(*slot);
+	rcu_read_unlock();
+}
+
+P2(int *slot2, int *fl, atomic_t *cnt)
+{
+	int r0;
+
+	r0 = READ_ONCE(*fl);
+	if (r0 == 1) {
+		atomic_set(cnt, 0);
+		smp_store_release(slot2, 1);
+	}
+}
+
+exists (0:r1=-1 /\ 2:r0=1 /\ 1:r0=1 /\ 1:r1=0 /\ 1:r2=1)
diff --git a/Documentation/litmus-tests/fs/file-ref-put-release-handoff.litmus b/Documentation/litmus-tests/fs/file-ref-put-release-handoff.litmus
new file mode 100644
index 000000000000..51c3e9d533a6
--- /dev/null
+++ b/Documentation/litmus-tests/fs/file-ref-put-release-handoff.litmus
@@ -0,0 +1,66 @@
+C file-ref-put-release-handoff
+
+(*
+ * Result: Sometimes
+ *
+ * file-ref-put-full-handoff.litmus with a release-ordered decrement,
+ * atomic_long_dec_return_release().  The release orders the cleared
+ * slot before the decrement itself and nothing more: not before the
+ * freelist store that hands the object to the other CPU, which is a
+ * later plain store.  So the other CPU can take the object, set ONEREF
+ * and publish it while the cleared slot is still invisible, and the
+ * lookup that incremented the recycled counter passes its recheck with
+ * the old slot and keeps a reference to a file the descriptor never
+ * named.  The deferred path of fput() gets such a barrier from the
+ * try_cmpxchg() of task_work_add(); the synchronous paths have nothing
+ * but the decrement.
+ *)
+
+{
+	atomic_t cnt = ATOMIC_INIT(0);
+	slot=1;
+	slot2=0;
+	fl=0;
+	0:r1=-9;
+}
+
+P0(int *slot, int *fl, atomic_t *cnt)
+{
+	int r0;
+	int r1;
+
+	WRITE_ONCE(*slot, 0);
+	r0 = atomic_fetch_sub_release(1, cnt);
+	if (r0 == 0) {
+		r1 = atomic_cmpxchg_release(cnt, -1, -4);
+		if (r1 == -1) {
+			WRITE_ONCE(*fl, 1);
+		}
+	}
+}
+
+P1(int *slot, atomic_t *cnt)
+{
+	int r0;
+	int r1;
+	int r2;
+
+	rcu_read_lock();
+	r0 = READ_ONCE(*slot);
+	r1 = atomic_fetch_add_acquire(1, cnt);
+	r2 = READ_ONCE(*slot);
+	rcu_read_unlock();
+}
+
+P2(int *slot2, int *fl, atomic_t *cnt)
+{
+	int r0;
+
+	r0 = READ_ONCE(*fl);
+	if (r0 == 1) {
+		atomic_set(cnt, 0);
+		smp_store_release(slot2, 1);
+	}
+}
+
+exists (0:r1=-1 /\ 2:r0=1 /\ 1:r0=1 /\ 1:r1=0 /\ 1:r2=1)
diff --git a/Documentation/litmus-tests/fs/file-ref-reload-dep-field.litmus b/Documentation/litmus-tests/fs/file-ref-reload-dep-field.litmus
new file mode 100644
index 000000000000..805d977f52ac
--- /dev/null
+++ b/Documentation/litmus-tests/fs/file-ref-reload-dep-field.litmus
@@ -0,0 +1,74 @@
+C file-ref-reload-dep-field
+
+(*
+ * Result: Never
+ *
+ * The recheck of __fget_files_rcu() passes when the file was recycled
+ * into the same descriptor: thread A closes fd and opens again, gets the
+ * same struct file and the same fd back, and thread B looks fd up
+ * meanwhile.  B's increment lands on the recycled file and the reload
+ * sees the slot hold that file again, which is fine, the descriptor
+ * does name it now.  But the fields of the file that B's caller reads
+ * next have to be the new incarnation's.  init_file() sets f_ref before
+ * do_dentry_open() stores f_op, and what publishes f_op is the release
+ * store of fd_install(), the one the reload saw.  So the field read has
+ * to depend on the reload, not on the first load.
+ *
+ * P0 is thread A: close() clears the slot, fput() marks the file DEAD
+ * and frees it, open() recycles it: init_file() stores ONEREF, the
+ * plain store of f_op follows, fd_install() publishes the slot.  P1 is
+ * thread B: the first load of the slot, the acquire-ordered increment,
+ * the reload and the compare, then f_op read through the reloaded
+ * pointer, as __get_file_rcu() does and as __fget_files_rcu() does since
+ * the reload became the pointer it returns.
+ *
+ * The slot holds a pointer to fop, the f_op field, or to q, NULL; fop
+ * is 1 in the old incarnation and 2 in the new one.  The address
+ * dependency of the field read on the reload orders it after the
+ * release store the reload read from, so it returns 2.
+ *)
+
+{
+	atomic_t cnt = ATOMIC_INIT(0);
+	slot=fop;
+	fop=1;
+	q=0;
+	0:r1=-9;
+	1:r3=-9;
+}
+
+P0(int **slot, int *fop, int *q, atomic_t *cnt)
+{
+	int r0;
+	int r1;
+
+	WRITE_ONCE(*slot, q);
+	r0 = atomic_fetch_sub(1, cnt);
+	if (r0 == 0) {
+		r1 = atomic_cmpxchg_release(cnt, -1, -4);
+		if (r1 == -1) {
+			atomic_set(cnt, 0);
+			WRITE_ONCE(*fop, 2);
+			smp_store_release(slot, fop);
+		}
+	}
+}
+
+P1(int **slot, atomic_t *cnt)
+{
+	int *r0;
+	int r1;
+	int *r2;
+	int r3;
+
+	rcu_read_lock();
+	r0 = rcu_dereference(*slot);
+	r1 = atomic_fetch_add_acquire(1, cnt);
+	r2 = rcu_dereference(*slot);
+	if (r2 == r0) {
+		r3 = READ_ONCE(*r2);
+	}
+	rcu_read_unlock();
+}
+
+exists (0:r1=-1 /\ 1:r0=fop /\ 1:r1=0 /\ 1:r2=fop /\ 1:r3=1)
diff --git a/Documentation/litmus-tests/fs/file-ref-reload-nodep-field.litmus b/Documentation/litmus-tests/fs/file-ref-reload-nodep-field.litmus
new file mode 100644
index 000000000000..a67c369b683e
--- /dev/null
+++ b/Documentation/litmus-tests/fs/file-ref-reload-nodep-field.litmus
@@ -0,0 +1,59 @@
+C file-ref-reload-nodep-field
+
+(*
+ * Result: Sometimes
+ *
+ * file-ref-reload-dep-field.litmus with f_op read through the pointer of
+ * the first load, which is what __fget_files_rcu() returned between
+ * 253ca8678d30 and the reload becoming the pointer it returns.  The
+ * field read is ordered after the increment by the acquire, and a fully
+ * ordered increment does no better, but nothing ties it to the reload
+ * and so nothing ties it to the release store of fd_install() that
+ * published the new f_op.  The lookup returns the recycled file and the
+ * caller reads the old incarnation's f_op.
+ *)
+
+{
+	atomic_t cnt = ATOMIC_INIT(0);
+	slot=fop;
+	fop=1;
+	q=0;
+	0:r1=-9;
+	1:r3=-9;
+}
+
+P0(int **slot, int *fop, int *q, atomic_t *cnt)
+{
+	int r0;
+	int r1;
+
+	WRITE_ONCE(*slot, q);
+	r0 = atomic_fetch_sub(1, cnt);
+	if (r0 == 0) {
+		r1 = atomic_cmpxchg_release(cnt, -1, -4);
+		if (r1 == -1) {
+			atomic_set(cnt, 0);
+			WRITE_ONCE(*fop, 2);
+			smp_store_release(slot, fop);
+		}
+	}
+}
+
+P1(int **slot, atomic_t *cnt)
+{
+	int *r0;
+	int r1;
+	int *r2;
+	int r3;
+
+	rcu_read_lock();
+	r0 = rcu_dereference(*slot);
+	r1 = atomic_fetch_add_acquire(1, cnt);
+	r2 = rcu_dereference(*slot);
+	if (r2 == r0) {
+		r3 = READ_ONCE(*r0);
+	}
+	rcu_read_unlock();
+}
+
+exists (0:r1=-1 /\ 1:r0=fop /\ 1:r1=0 /\ 1:r2=fop /\ 1:r3=1)

-- 
2.53.0

[PATCH 5/5] Documentation: update the lock-free file lookup in files.rst

[Christian Brauner]

The lock-free lookup section of files.rst still sends readers to the
lookup_fdget_rcu() helpers that commit 8fd3395ec905 ("get rid of
...lookup...fdget_rcu() family") removed. And the SLAB_TYPESAFE_BY_RCU
paragraph still describes the reference as atomic_long_inc_not_zero()
and points to __files_get_rcu(), which doesn't exist, the lookup is
__fget_files_rcu(). Files have been using file_ref since commit
90ee6ed776c0 ("fs: port files to file_ref").

So point readers to fget() and friends and spell out what the lookup
does now: take the reference with file_ref_get(), reload the pointer,
compare against a copy the compiler can't see through and return the
reloaded pointer. Then say which orderings that rests on, the acquire
in file_ref_get(), the dependency on the reload and the full ordering
of file_ref_put(), and where they are documented and checked:
include/linux/file_ref.h and Documentation/litmus-tests/fs/.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 Documentation/filesystems/files.rst | 48 +++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/Documentation/filesystems/files.rst b/Documentation/filesystems/files.rst
index eb770f891b27..59536e62ed74 100644
--- a/Documentation/filesystems/files.rst
+++ b/Documentation/filesystems/files.rst
@@ -61,29 +61,32 @@ the fdtable structure -
 3. For any update to the fd table, files->file_lock must
    be held.
 
-4. To look up the file structure given an fd, a reader
-   must use either lookup_fdget_rcu() or files_lookup_fdget_rcu() APIs. These
-   take care of barrier requirements due to lock-free lookup.
+4. To look up the file structure given an fd, a reader must use one of
+   the fget() helpers: fget(), fget_raw(), fget_task(), fget_task_next()
+   or fdget() and friends. These take the reference and take care of
+   the barrier requirements of the lock-free lookup.
 
    An example::
 
 	struct file *file;
 
-	rcu_read_lock();
-	file = lookup_fdget_rcu(fd);
-	rcu_read_unlock();
+	file = fget(fd);
 	if (file) {
 		...
-                fput(file);
+		fput(file);
 	}
 	....
 
+   fdget() skips the reference when the file descriptor table isn't
+   shared, see the comment above __fget_light() for the rules that come
+   with that. Under files->file_lock a file can be looked up without a
+   reference with files_lookup_fd_locked() as long as the lock is held.
+
 5. Since both fdtable and file structures can be looked up
    lock-free, they must be installed using rcu_assign_pointer()
    API. If they are looked up lock-free, rcu_dereference()
    must be used. However it is advisable to use files_fdtable()
-   and lookup_fdget_rcu()/files_lookup_fdget_rcu() which take care of these
-   issues.
+   and the fget() helpers which take care of these issues.
 
 6. While updating, the fdtable pointer must be looked up while
    holding files->file_lock. If ->file_lock is dropped, then
@@ -105,15 +108,24 @@ the fdtable structure -
    Since locate_fd() can drop ->file_lock (and reacquire ->file_lock),
    the fdtable pointer (fdt) must be loaded after locate_fd().
 
-On newer kernels rcu based file lookup has been switched to rely on
-SLAB_TYPESAFE_BY_RCU instead of call_rcu(). It isn't sufficient anymore
-to just acquire a reference to the file in question under rcu using
-atomic_long_inc_not_zero() since the file might have already been
-recycled and someone else might have bumped the reference. In other
-words, callers might see reference count bumps from newer users. For
-this is reason it is necessary to verify that the pointer is the same
-before and after the reference count increment. This pattern can be seen
-in get_file_rcu() and __files_get_rcu().
+Files are allocated from a SLAB_TYPESAFE_BY_RCU cache since commit
+0ede61d8589c ("file: convert to SLAB_TYPESAFE_BY_RCU"). Freeing a file
+doesn't wait for a grace period anymore. The memory can be handed out
+again right away and only the type of the object is stable under
+rcu_read_lock(). So it isn't sufficient to take a reference on the file
+found in a descriptor slot. The file might have been closed, freed and
+recycled in the meantime and the reference lands on whoever owns the
+object now. The lock-free lookup in __fget_files_rcu() and
+__get_file_rcu() therefore:
+
+(1) loads the pointer from the slot and takes a reference with
+    file_ref_get(), which fails if the file is dead,
+
+(2) reloads the pointer and compares it against the earlier load, if they
+    differ the file was recycled and the reference is dropped again, and
+
+(3) returns the reloaded pointer so that everything the caller reads
+    from the file depends on the reload.
 
 In addition, it isn't possible to access or check fields in struct file
 without first acquiring a reference on it under rcu lookup. Not doing

-- 
2.53.0