store futexes in per-allocation data rather than globally

[RalfJung]

This is a change in behavior. We used to index futexes entirely on their absolute address, so they stuck around even if the allocation backing that address disappears -- in fact one could signal FUTEX_WAKE on unallocated memory. That works fine on Linux but is quite odd from a Rust AM perspective. It also means Miri has a leak, as some futex state never disappears.

So this PR attaches the futex to the "virtual" address inside an allocation, meaning it disappears when the allocation is freed. Strictly speaking this means we will miss wakeups if the address gets reused, and then someone does FUTEX_WAKE and expects to wake up a thread that started blocking on the old address before it got deallocated... but that seems really contrived?

@rust-lang/miri what do you think?

[bors]

☔ The latest upstream changes (presumably #3973) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #3974) made this pull request unmergeable. Please resolve the merge conflicts.

[saethlin]

in fact one could signal FUTEX_WAKE on unallocated memory. That works fine on Linux but is quite odd from a Rust AM perspective

I don't mind declaring such cases Too Weird For Rust (or Miri). Same goes for reusing addresses; if someone comes up with an actual use case for these we can consider figuring it out then. But that seems unlikely.

[RalfJung]

Cc @m-ou-se who did the original implementation here and made it work on al (even dangling) addresses.

[m-ou-se]

The only thing that's important is that it doesn't segfault/crash/panic/do anything bad when calling futex_wake on a futex that no longer exists.

So, something like:

let b = Box::new(AtomicU32::new(0));
let p = b.as_ptr();
// ...
drop(b);
// ...
futex_wake(p, 1); // "use after free" is perfectly fine for futex_wake!

This should just do nothing (or spuriously wake another futex, whatever).

[m-ou-se]

This should still work. At least parking_lot depends on "wake after free" to be acceptable.

Edit: or it should return -1 with errno set to EFAULT, treating deallocated memory as unmapped memory.

[m-ou-se]

From parking_lot_core:

        // The thread data may have been freed at this point, but it doesn't
        // matter since the syscall will just return EFAULT in that case.
        let r = libc::syscall(
            libc::SYS_futex,
            self.futex,
            libc::FUTEX_WAKE | libc::FUTEX_PRIVATE_FLAG,
            1,
        );

Note that the linux kernel returns EFAULT when the memory is unmapped. In most cases, memory freed by the global allocator is still mapped (so the kernel doesn't know it's deallocated), but it seems perfectly fine for miri to just always return EFAULT if the allocation is gone.

[RalfJung]

Returning EFAULT would for the first time let a program "test" whether a given address is allocated. But maybe we should still do it.

[m-ou-se]

Returning EFAULT would for the first time let a program "test" whether a given address is allocated. But maybe we should still do it.

Yeah. It's more accurate to just return success if the memory is deallocated, without waking up anything. I don't think it matters much.

[RalfJung]

It's more accurate to just return success if the memory is deallocated, without waking up anything.

Yeah I think I prefer that.

EDIT: After thinking about it some more, I think I prefer the EFAULT variant. That makes it more likely that someone will file a bug report if this behavior for some reason doesn't work for them.

[RalfJung]

I've also changed the ordering of the read in futex_wait to be "acquire" -- it only seems reasonable that if we observe a value this way, we actually establish proper ordering with it. (In practice this is a syscall so should definitely have this "acquire" effect.)