diff mbox series

fs: add memory barrier in __fget_light()

Message ID 20221031171307.2784981-1-jannh@google.com
State New
Headers
Series fs: add memory barrier in __fget_light() |

Commit Message

Jann Horn Oct. 31, 2022, 5:13 p.m. UTC 
  We must prevent the CPU from reordering the files->count read with the
FD table access like this, on architectures where read-read reordering is
possible:

    files_lookup_fd_raw()
                                  close_fd()
                                  put_files_struct()
    atomic_read(&files->count)

I would like to mark this for stable, but the stable rules explicitly say
"no theoretical races", and given that the FD table pointer and
files->count are explicitly stored in the same cacheline, this sort of
reordering seems quite unlikely in practice...

If this is too expensive on platforms like arm64, I guess the more
performant alternative would be to add another flags field that tracks
whether the fs_struct was ever shared and check that instead of the
reference count in __fget_light().

Signed-off-by: Jann Horn <jannh@google.com>
---
 fs/file.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)


base-commit: 30a0b95b1335e12efef89dd78518ed3e4a71a763

Comments

Linus Torvalds Oct. 31, 2022, 5:37 p.m. UTC | #1 
On Mon, Oct 31, 2022 at 10:13 AM Jann Horn <jannh@google.com> wrote:
>
> If this is too expensive on platforms like arm64, I guess the more
> performant alternative would be to add another flags field that tracks
> whether the fs_struct was ever shared and check that instead of the
> reference count in __fget_light().

No, the problem is that you should never use the "smp_*mb()" horrors
for any new code.

All the "smp_*mb()" things really are broken. Please consider them
legacy garbage. It was how people though about SMP memory ordering in
the bad old days.

So get with the 21st century, and instead replace the "atomic_read()"
with a "smp_load_acquire()".

Much better on sane architectures.

                           Linus
Mark Rutland Nov. 1, 2022, 10:53 a.m. UTC | #2 
On Mon, Oct 31, 2022 at 10:37:01AM -0700, Linus Torvalds wrote:
> On Mon, Oct 31, 2022 at 10:13 AM Jann Horn <jannh@google.com> wrote:
> >
> > If this is too expensive on platforms like arm64, I guess the more
> > performant alternative would be to add another flags field that tracks
> > whether the fs_struct was ever shared and check that instead of the
> > reference count in __fget_light().
> 
> No, the problem is that you should never use the "smp_*mb()" horrors
> for any new code.
> 
> All the "smp_*mb()" things really are broken. Please consider them
> legacy garbage. It was how people though about SMP memory ordering in
> the bad old days.
> 
> So get with the 21st century, and instead replace the "atomic_read()"
> with a "smp_load_acquire()".

Minor nit: atomic{,64,_long}_{read_acquire,set_release}() exist to be used
directly on atomics and should d.t.r.t. on all architectures (e.g. where 64-bit
atomics on 32-bit platforms have extra requirements).

So this instance can be:

  ...
  if (atomic_read_acquire(&files->count) == 1) {
  ...

Mark.
diff mbox series

Patch

diff --git a/fs/file.c b/fs/file.c
index 5f9c802a5d8d3..6144287ddc0fe 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -1004,6 +1004,18 @@  static unsigned long __fget_light(unsigned int fd, fmode_t mask)
 	struct file *file;
 
 	if (atomic_read(&files->count) == 1) {
+		/*
+		 * If another thread is concurrently calling close_fd() followed
+		 * by put_files_struct(), we must not observe the old table
+		 * entry combined with the new refcount - otherwise we could
+		 * return a file that is concurrently being freed.
+		 *
+		 * Pairs with atomic_dec_and_test() in put_files_struct().
+		 * An alternative to using a barrier here would be to use a
+		 * separate field in files_struct to track whether it was ever
+		 * shared.
+		 */
+		smp_rmb();
 		file = files_lookup_fd_raw(files, fd);
 		if (!file || unlikely(file->f_mode & mask))
 			return 0;