| Message ID | 20221031171307.2784981-1-jannh@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs: add memory barrier in __fget_light() | expand |
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
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 --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;
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