Don't reprotect more than once when linking code

RESOLVED FIXED

Status

()

Core
JavaScript Engine: JIT
P5
normal
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Created attachment 8700655 [details] [diff] [review]
Patch

We have an AutoWritableJitCode in Linker::newCode. Most callers have to update the JIT code after that, so they have to use another AutoWritableJitCode.

This patch adds a Maybe<AutoWritableJitCode> to the Linker class, so the code is writable as long as the Linker is on the stack. This allows us to remove some AutoWritableJitCode instances.

Ion IC stubs are even worse, we used AutoWritableJitCode for the new stub like 3 or 4 times. This patch refactors the code a bit so we finish the stub completely in linkCode, while it's still writable, and then we attach it.
Attachment #8700655 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8700655 [details] [diff] [review]
Patch

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

Nice work!

::: js/src/jit/Ion.cpp
@@ +747,5 @@
>  JitCompartment::toggleBarriers(bool enabled)
>  {
>      // Toggle barriers in compartment wide stubs that have patchable pre barriers.
>      if (regExpExecStub_)
> +        regExpExecStub_->togglePreBarriers(enabled, Reprotect);

Wouldn't it be better to use

  jrt->execAlloc().makeAllWritable();
  jrt->execAlloc().makeAllExecutable();

in this function?
Attachment #8700655 - Flags: review?(nicolas.b.pierron) → review+

Comment 2

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fb5de3f44c3
(Assignee)

Comment 3

2 years ago
Keeping this bug open because makeAllWritable/makeExecutable are added in a patch I didn't land yet, so I still have to land that part. I pushed this patch to see how it affects AWFY.

(In reply to Nicolas B. Pierron [:nbp] from comment #1)
> Wouldn't it be better to use
> 
>   jrt->execAlloc().makeAllWritable();
>   jrt->execAlloc().makeAllExecutable();
> 
> in this function?

Good suggestion. I'm not sure, I think it depends on the number of Zones. It may indeed be more efficient to toggle all pages in the runtime, or we could use a separate ExecutableAllocator for each Zone... I'll try to do some measurements.

Also, makeAllWritable/makeAllExecutable are less secure, because an attacker could use web workers to write to JIT code while it's writable on the main thread. Still a lot safer than RWX.
Keywords: leave-open

Comment 4

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1fb5de3f44c3

Comment 5

9 months ago
Jan, did the second patch land yet?
Flags: needinfo?(jdemooij)
Priority: -- → P5
(Assignee)

Comment 6

9 months ago
(In reply to Jon Coppeard (:jonco) from comment #5)
> Jan, did the second patch land yet?

I think we can mark this FIXED and don't need the second patch.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
Flags: needinfo?(jdemooij)
Resolution: --- → FIXED
(Assignee)

Updated

9 months ago
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.