Closed Bug 1437483 Opened 2 years ago Closed 2 years ago

Spectre mitigations for Ion object type barriers

Categories

(Core :: JavaScript Engine: JIT, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

No description provided.
Depends on: 1437862
Priority: -- → P1
This adds a |spectre.object-mitigations.barriers| pref.

For perf testing etc, I think it makes sense to have different prefs for at least TI and IC object type mitigations; so probably 3 prefs or so in the end which doesn't seem too bad.
Attachment #8953002 - Flags: review?(nicolas.b.pierron)
This depends on the clean up patches in bug 1437862.

The main changes:

* Removes MMonitorTypes/LMonitorTypes. This instruction is very similar to MTypeBarrier and MTypeBarrier is more optimized, so we should just use the latter.

* MTypeBarrier lowering now uses defineReuseInput intead of redefineInput, if needed, to ensure all uses see the updated Value/object on speculative paths.

* guardObjectType/guardTypeSet take a spectreRegToZero argument, a register that will be set to zero if needed. Which register to pass depends on the caller - usually it's the value/payload reg we're type checking, but for Ion function prologue arguments checks we zero the stack pointer because all these Value live on the stack.

I considered a few other approaches but this one seems simplest and most efficient.

In terms of performance, I don't think this affects Speedometer/Sunspider/Kraken much. It may regress Octane-deltablue a bit - I want to see what AWFY says. It's possible we could elide TypeBarriers in some more cases.

Ion has a lot of these barriers and if perf of this is okay, the overhead of other object mitigations will hopefully be acceptable too.
Attachment #8953006 - Flags: review?(nicolas.b.pierron)
Attachment #8953006 - Flags: review?(luke)
I'll probably land this separately.
Attachment #8953011 - Flags: review?(luke)
Comment on attachment 8953002 [details] [diff] [review]
Part 1 - Add pref

Review of attachment 8953002 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/ContentPrefs.cpp
@@ +128,5 @@
>    "javascript.options.native_regexp",
>    "javascript.options.parallel_parsing",
>    "javascript.options.shared_memory",
>    "javascript.options.spectre.index_masking",
> +  "javascript.options.spectre.object_mitigations.barriers",

I wonder if we should make a unique spectre pref here, as from what I understand this is command line space, which can be quite limited on some systems.
Attachment #8953002 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8953006 [details] [diff] [review]
Part 2 - Mitigate guardTypeSet/guardObjectType

Review of attachment 8953006 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/MIR.cpp
@@ +2366,5 @@
> +    // LTypeBarrier does not need its own def usually, because we can use the
> +    // input's allocation (LIRGenerator::redefineInput). However, if Spectre
> +    // mitigations are enabled, guardObjectType may zero the object register on
> +    // speculatively executed paths, so LTypeBarrier needs to have its own def
> +    // then to guarantee all uses will see this potentially-zeroed value.

I am really not sure I understand.  From what I understood redefineInput will just use the same vreg, while defineReuseInput will use another vreg but bound to the same register.

So I am not sure to understand this argument, especially since MTypeBarrier is part of the data-flow, and as such independently of any reused registers, any wrong speculative execution would zero the payload/object value.

So I do not understand the need to have so much code to decide whether we have a (or not) vreg for the LTypeBarrier{V,O}, either should be fine?

@@ +6664,5 @@
> +    if (ins->type() == MIRType::Undefined) {
> +        ins = MConstant::New(alloc, UndefinedValue());
> +        current->add(ins);
> +    } else if (ins->type() == MIRType::Null) {
> +        ins = MConstant::New(alloc, NullValue());

Why is that necessary? Don't we only care about types which might be interpreted as a pointer?
(In reply to Nicolas B. Pierron [:nbp] {backlog: ~41} from comment #5)
> So I do not understand the need to have so much code to decide whether we
> have a (or not) vreg for the LTypeBarrier{V,O}, either should be fine?

Let's say we have MGetPropertyCache -> MTypeBarrier.

With redefineInput, the register allocator is allowed to do this:

  - LGetPropertyCacheV -> rax
  - LMoveGroup rax -> stack:8
  - LTypeBarrierV rax
  ...
  - LMoveGroup stack:8 -> rax
  - LFoo rax

So even if we zero |rax| in LTypeBarrierV, there's nothing preventing us from reading the "unpoisoned" value from the stack if it was spilled there. With defineReuseInput we tell the register allocator we might change the Value, so it's no longer allowed to do this.

Does that make sense?

> Why is that necessary? Don't we only care about types which might be
> interpreted as a pointer?

MTypeBarrier's TypePolicy asserts we have no uses if the barrier's type is Undefined/Null, so we have to substitute a constant (we also do this in other places in IonBuilder where we add MTypeBarrier).
(In reply to Jan de Mooij [:jandem] from comment #6)
> (In reply to Nicolas B. Pierron [:nbp] {backlog: ~41} from comment #5)
> > So I do not understand the need to have so much code to decide whether we
> > have a (or not) vreg for the LTypeBarrier{V,O}, either should be fine?
> 
> Let's say we have MGetPropertyCache -> MTypeBarrier.
> 
> With redefineInput, the register allocator is allowed to do this:
> 
>   - LGetPropertyCacheV -> rax
>   - LMoveGroup rax -> stack:8
>   - LTypeBarrierV rax
>   ...
>   - LMoveGroup stack:8 -> rax
>   - LFoo rax
> 
> So even if we zero |rax| in LTypeBarrierV, there's nothing preventing us
> from reading the "unpoisoned" value from the stack if it was spilled there.
> With defineReuseInput we tell the register allocator we might change the
> Value, so it's no longer allowed to do this.
> 
> Does that make sense?

Yes, it does.
Comment on attachment 8953006 [details] [diff] [review]
Part 2 - Mitigate guardTypeSet/guardObjectType

Review of attachment 8953006 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/shared/Lowering-shared-inl.h
@@ +172,5 @@
> +    MOZ_ASSERT(mir->type() == MIRType::Value);
> +
> +    uint32_t vreg = getVirtualRegister();
> +
> +#if JS_BITS_PER_WORD == 32

nit: #ifdef JS_NUNBOX32
Attachment #8953006 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8953006 - Flags: review?(luke) → review+
Attachment #8953011 - Flags: review?(luke) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c34b095d74f1
part 1 - Add pref for Spectre mitigations for Ion object type barriers. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/594ed5cfd631
part 2 - Spectre mitigations for guardObjectType, disabled by default. r=nbp,luke
Keywords: leave-open
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bfa2571aade
follow-up - Use mozilla::Unused to fix warnings about unused temps on ARM64. r=red CLOSED TREE
Since this issue is still open I'll post this here. Stub to get mips32/mips64 build working again. Please land after review.
Attachment #8954276 - Flags: review?(jdemooij)
Comment on attachment 8954276 [details] [diff] [review]
bug1437483_mips_stubs.patch

Review of attachment 8954276 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

(Next time please file a new bug; I'll land this now to not confuse the sheriffs with checkin-needed.)
Attachment #8954276 - Flags: review?(jdemooij) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e2abc1dfda7
part 3 - Enable Ion object type barrier mitigations by default. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9c1325f674a
part 4 - Implement spectreMovePtr for MIPS. r=jandem
Keywords: leave-open
So according to AWFY, this regressed Octane {DeltaBlue, EarleyBoyer, Gameboy} each by 8-10%. Total score regressed by ~2-3%.

An old heuristic for CALLELEM is biting us on Gameboy; I think we can remove that code and it should give us better type information and fix the regression because we can skip more prologue argument type checks.

I also think we can try harder to elide prologue type barriers for polymorphic calls. Hopefully that will help Deltablue a bit.
And in generateArgumentsChecks we should probably zero a temp register instead of the stack pointer and then at the end MOV/AND that with the stack pointer, so we don't have as many dependency chains when there are multiple object arguments. I'll check if that's indeed faster.

More patches coming up (in other bugs).
Blocks: 1441587
https://hg.mozilla.org/mozilla-central/rev/0e2abc1dfda7
https://hg.mozilla.org/mozilla-central/rev/b9c1325f674a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.