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...
Signed-off-by: Jann Horn <[email protected]>
---
fs/file.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/fs/file.c b/fs/file.c
index 5f9c802a5d8d3..c942c89ca4cda 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -1003,7 +1003,16 @@ static unsigned long __fget_light(unsigned int fd, fmode_t mask)
struct files_struct *files = current->files;
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.
+ *
+ * atomic_read_acquire() pairs with atomic_dec_and_test() in
+ * put_files_struct().
+ */
+ if (atomic_read_acquire(&files->count) == 1) {
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
On Mon, Oct 31, 2022 at 06:52:56PM +0100, Jann Horn wrote:
> 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...
Looks sane, but looking at the definition of atomic_read_acquire... ouch.
static __always_inline int
atomic_read_acquire(const atomic_t *v)
{
instrument_atomic_read(v, sizeof(*v));
return arch_atomic_read_acquire(v);
}
OK...
; git grep -n -w arch_atomic_read_acquire
include/linux/atomic/atomic-arch-fallback.h:220:#ifndef arch_atomic_read_acquire
include/linux/atomic/atomic-arch-fallback.h:222:arch_atomic_read_acquire(const atomic_t *v)
include/linux/atomic/atomic-arch-fallback.h:235:#define arch_atomic_read_acquire arch_atomic_read_acquire
include/linux/atomic/atomic-instrumented.h:35: return arch_atomic_read_acquire(v);
include/linux/atomic/atomic-long.h:529: return arch_atomic_read_acquire(v);
No arch-specific instances, so...
static __always_inline int
arch_atomic_read_acquire(const atomic_t *v)
{
int ret;
if (__native_word(atomic_t)) {
ret = smp_load_acquire(&(v)->counter);
} else {
ret = arch_atomic_read(v);
__atomic_acquire_fence();
}
return ret;
}
OK, but when would that test not be true? We have unconditional
typedef struct {
int counter;
} atomic_t;
and
#define __native_word(t) \
(sizeof(t) == sizeof(char) || sizeof(t) == sizeof(short) || \
sizeof(t) == sizeof(int) || sizeof(t) == sizeof(long))
Do we really have any architectures where a structure with one
int field does *not* have a size that would satisfy that check?
Is it future-proofing for masturbation sake, or am I missing something
real here?
On Mon, Oct 31, 2022 at 7:08 PM Al Viro <[email protected]> wrote:
[...]
> No arch-specific instances, so...
> static __always_inline int
> arch_atomic_read_acquire(const atomic_t *v)
> {
> int ret;
>
> if (__native_word(atomic_t)) {
> ret = smp_load_acquire(&(v)->counter);
> } else {
> ret = arch_atomic_read(v);
> __atomic_acquire_fence();
> }
>
> return ret;
> }
[...]
> Do we really have any architectures where a structure with one
> int field does *not* have a size that would satisfy that check?
>
> Is it future-proofing for masturbation sake, or am I missing something
> real here?
include/linux/atomic/atomic-arch-fallback.h has a comment at the top that says:
// Generated by scripts/atomic/gen-atomic-fallback.sh
// DO NOT MODIFY THIS FILE DIRECTLY
On Mon, Oct 31, 2022 at 07:13:30PM +0100, Jann Horn wrote:
> On Mon, Oct 31, 2022 at 7:08 PM Al Viro <[email protected]> wrote:
> [...]
> > No arch-specific instances, so...
> > static __always_inline int
> > arch_atomic_read_acquire(const atomic_t *v)
> > {
> > int ret;
> >
> > if (__native_word(atomic_t)) {
> > ret = smp_load_acquire(&(v)->counter);
> > } else {
> > ret = arch_atomic_read(v);
> > __atomic_acquire_fence();
> > }
> >
> > return ret;
> > }
> [...]
> > Do we really have any architectures where a structure with one
> > int field does *not* have a size that would satisfy that check?
> >
> > Is it future-proofing for masturbation sake, or am I missing something
> > real here?
>
> include/linux/atomic/atomic-arch-fallback.h has a comment at the top that says:
>
> // Generated by scripts/atomic/gen-atomic-fallback.sh
> // DO NOT MODIFY THIS FILE DIRECTLY
Hmm... Apparently, the source is shared for atomic and atomic64, and the
check is intended for atomic64 counterpart of that thing on 32bit boxen.
Might make sense to put a comment in there...
The question about architectures with non-default implementations still
stands, though.
Anyway, it's unrelated to the patch itself. I'm fine with it in the current
form. Will apply for the next merge window, unless Linus wants it in right
now.
On Mon, Oct 31, 2022 at 11:08 AM Al Viro <[email protected]> wrote:
>
> Looks sane, but looking at the definition of atomic_read_acquire... ouch.
The compiler should sort all that out and the mess shouldn't affect
any code generation.
But I also wouldn't mind somebody fixing things up, because I do agree
that checking whether 'atomic_t' is a native word size is kind of
pointless and probably just makes our build times unnecessarily
longer.
Linus
On Mon, Oct 31, 2022 at 11:48 AM Al Viro <[email protected]> wrote:
>
> Anyway, it's unrelated to the patch itself. I'm fine with it in the current
> form. Will apply for the next merge window, unless Linus wants it in right
> now.
It doesn't strike me as hugely critical, so I'm fine with it being put
in any random pile of "fixes to be applied" as long as it doesn't get
lost entirely. But if y ou have a "fixes" branch that may end up
coming to me before this release is over, that's not the wrong place
either.
I would tend to agree with Jann that the re-ordering doesn't look very
likely because it's the same cacheline, so even an aggressively
out-of-order core really doesn't seem to be very likely to trigger any
issues. So you have a really unlikely situation to begin with, and
even less reason for it then triggering the re-ordering.
The "original situation is unlikely" can probably be made quite likely
with an active attack, but that active attacker would then also also
have to rely on "that re-ordering looks sketchy", and actively hunt
for hardware where it can happen.
And said hardware may simply not exist, even if the race is certainly
theoretically possible on any weakly ordered CPU.
Linus
On Mon, Oct 31, 2022 at 12:07:36PM -0700, Linus Torvalds wrote:
> On Mon, Oct 31, 2022 at 11:48 AM Al Viro <[email protected]> wrote:
> >
> > Anyway, it's unrelated to the patch itself. I'm fine with it in the current
> > form. Will apply for the next merge window, unless Linus wants it in right
> > now.
>
> It doesn't strike me as hugely critical, so I'm fine with it being put
> in any random pile of "fixes to be applied" as long as it doesn't get
> lost entirely. But if y ou have a "fixes" branch that may end up
> coming to me before this release is over, that's not the wrong place
> either.
Applied to #fixes and pushed out...