[PATCH] fs: add memory barrier in __fget_light()

[PATCH] fs: add memory barrier in __fget_light()

[Jann Horn]

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(+)

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;

base-commit: 30a0b95b1335e12efef89dd78518ed3e4a71a763
-- 
2.38.1.273.g43a17bfeac-goog

Re: [PATCH] fs: add memory barrier in __fget_light()

[Linus Torvalds]

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

Re: [PATCH] fs: add memory barrier in __fget_light()

[Mark Rutland]

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.