NJ needs to mark code pages as R+X when done

VERIFIED FIXED in flash10.1

Status

P2
normal
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: stejohns, Assigned: edwsmith)

Tracking

unspecified
flash10.1
Bug Flags:
in-testsuite ?
flashplayer-qrb +
flashplayer-triage +

Details

(Whiteboard: fixed-in-nanojit, fixed-in-tamarin, fixed-in-tracemonkey)

Attachments

(4 obsolete attachments)

(Reporter)

Description

10 years ago
Currently NJ always marks code pages as R+W+X. It really needs to set permission to R+W during code generation, then R+X when done.

Updated

10 years ago
Flags: flashplayer-triage+
Flags: flashplayer-qrb?

Updated

10 years ago
Flags: flashplayer-qrb? → flashplayer-qrb+
Priority: -- → P3
Target Milestone: --- → flash10.x

Updated

9 years ago
Priority: P3 → P4

Updated

9 years ago
Priority: P4 → P2
(Assignee)

Comment 1

9 years ago
related:  Bug 506693

Updated

9 years ago
Assignee: nobody → edwsmith

Updated

9 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

9 years ago
Created attachment 420405 [details] [diff] [review]
Mark code chunks RW as they are allocated and RX after finishing jitting.

First prototype.  We allocate code 64K at a time, each time we have to touch a 64K page, unprotect it, and then at the end of jitting each method, mark each touched page RX.
(Assignee)

Updated

9 years ago
Depends on: 538393
(Assignee)

Updated

9 years ago
Attachment #420405 - Attachment is obsolete: true
(Assignee)

Comment 3

9 years ago
Created attachment 420579 [details] [diff] [review]
Adds protect/unprotect API to CodeAlloc

Removes calls to VMPI_setPageProtection from CodeAlloc, and adds a new protect/unprotect API to CodeAlloc, along with an SPI for the vm to implement actual page protection.

It is up to the VM to call codeAlloc->protect() before executing jit'd code, but CodeAlloc will internally call unprotect() before modifying blocks, as code is generated.

A flag per code chunk is used so that only modified pages are unprotected and reprotected.
(Assignee)

Comment 4

9 years ago
Created attachment 420580 [details] [diff] [review]
Use the CodeAlloc protection api to remove W on executable pages.

Implements CodeAlloc.[un]protectCodeChunk() two ways, controlled by the AVMPLUS_PROTECT_JITMEM feature.  With the feature disabled, code memory is RWX all the time.  With it enabled, pages are RW when generating code and RX when executing.

Initial testing shows aproximatley 10% slower startup time with AVMFEATURE_PROTECT_JITMEM=1 compared to =0, due to the extra page protection calls.
(Assignee)

Updated

9 years ago
Attachment #420579 - Flags: review?(rreitmai)
(Assignee)

Updated

9 years ago
Attachment #420580 - Flags: review?(rreitmai)

Comment 5

9 years ago
Comment on attachment 420579 [details] [diff] [review]
Adds protect/unprotect API to CodeAlloc

Does unprotect() cover the case where a page is partially filled with executable code (which with background compiling could even be running at the time)
and we're wish to add instructions to it?
Attachment #420579 - Flags: review?(rreitmai) → review+

Comment 6

9 years ago
Comment on attachment 420580 [details] [diff] [review]
Use the CodeAlloc protection api to remove W on executable pages.

r+, assuming above comment is addressed
Attachment #420580 - Flags: review?(rreitmai) → review+
(Assignee)

Comment 7

9 years ago
the whole patch assumes single-threading, i.e. that no page being protected or unprotected has other executing threads on it.

Comment 8

9 years ago
That makes it easier; so in the case of an OOM failure what state will the pages be left in.

I.e. assume that we're compiling code (i.e we've marked all pages involved as 'RW') and the allocator cannot obtain any more memory for the JIT, will the pages remain as 'RW' or are we able to mark pages back to 'RX'.

Or is it assumed that on OOM we'll flush all JIT'd code and if so, do we have code in place to enforce this policy.
(Assignee)

Updated

9 years ago
Attachment #420579 - Flags: superreview?(nnethercote)
(Assignee)

Comment 9

9 years ago
In case of an OOM failure, the state of CodeAlloc is however it was left, so a recovering host could call CodeAlloc::protect() which would re-protect each touched page.
Comment on attachment 420579 [details] [diff] [review]
Adds protect/unprotect API to CodeAlloc

I'm not sure about the performance implications and the single-threaded requirement, whether they are a problem for TM.  If it's off by default that's less of an issue but I'll defer to Gal here.

Nit:  I find the words "protect" and "unprotect" entirely unintuitive here.  "markRX" and "markRW" would be better.
Attachment #420579 - Flags: superreview?(nnethercote) → superreview?(gal)
(Assignee)

Comment 11

9 years ago
how about markExec() and markWrite(); the implementation of these methods is up to the host vm which may not use RX and RW, but instead make code RWX all the time and make these methods no-ops.

Comment 12

9 years ago
Comment on attachment 420579 [details] [diff] [review]
Adds protect/unprotect API to CodeAlloc

The patch leaves us a choice whether we want to take the runtime hit or not. We protect/unprotect repeatedly as we run code since we extend code all the time. Our runtime overhead might be thus larger. On the other hand, we do compile less than tamarin on startup. Lets wire this up in TM and see how it goes. I think its a good enough starting point.
Attachment #420579 - Flags: superreview?(gal) → superreview+
"exec" and "write" are not as immediately intuitive to me as the spelled-out forms markExecutable() and markWritable(), mostly because neither "exec" nor "write" is an adjective.
(Assignee)

Comment 14

9 years ago
Comment on attachment 420579 [details] [diff] [review]
Adds protect/unprotect API to CodeAlloc

pushed to nanojit-central
http://hg.mozilla.org/projects/nanojit-central/rev/a1002278492b
Attachment #420579 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Whiteboard: fixed-in-nanojit
(Assignee)

Comment 15

9 years ago
Comment on attachment 420580 [details] [diff] [review]
Use the CodeAlloc protection api to remove W on executable pages.

pushed to tamarin-redux
http://hg.mozilla.org/tamarin-redux/rev/137e096d78fa
Attachment #420580 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tamarin
http://hg.mozilla.org/tracemonkey/rev/69d5a3454a6e
Whiteboard: fixed-in-nanojit, fixed-in-tamarin → fixed-in-nanojit, fixed-in-tamarin, fixed-in-tracemonkey
This broke TM on Windows (WinCE?).  *All* of the VMPI_setPageProtection() calls in avmplus.cpp are bogus, the refer to non-existent variables.  So I backed it out:

http://hg.mozilla.org/projects/nanojit-central/rev/f743ffa4673f

Unfortunately backing it out involved a merge and now the update-nanojit script isn't working for TM, so I am unable to back it out of TM.  I don't know if I did something wrong or if this is a shortcoming of update-nanojit.
I think I know how to fix this now, but I need to check with Graydon (via email) because I don't want to break things even more.
(Assignee)

Comment 19

9 years ago
Created attachment 422735 [details] [diff] [review]
(do-over) Adds page protection SPI, and fixes bogus VMPI_setPageProtection calls from the first patch

Applies to nanojit-central tip
Attachment #422735 - Flags: review?(nnethercote)
Backed out (with Graydon's help) from TM:

http://hg.mozilla.org/tracemonkey/rev/d85d14f9ff3d
Patch looks good, but I'll run it on the TM try server before giving r+ :)
Comment on attachment 422735 [details] [diff] [review]
(do-over) Adds page protection SPI, and fixes bogus VMPI_setPageProtection calls from the first patch

TM try server results look good enough.
Attachment #422735 - Flags: review?(nnethercote) → review+
(Assignee)

Comment 23

9 years ago
Comment on attachment 422735 [details] [diff] [review]
(do-over) Adds page protection SPI, and fixes bogus VMPI_setPageProtection calls from the first patch

pushed to n-c
http://hg.mozilla.org/projects/nanojit-central/rev/3c9030f46c15
Attachment #422735 - Attachment is obsolete: true

Comment 25

9 years ago
http://hg.mozilla.org/mozilla-central/rev/69d5a3454a6e
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
QA Contact: nanojit → dschaffe

Updated

9 years ago
QA Contact: dschaffe → brbaker

Comment 26

9 years ago
Edwin does this need/warrant a testcase? If not I will remove the in-testsuite? and verify/fixed this bug as it has been pushed into all necessary repos
Flags: in-testsuite?
(Assignee)

Comment 27

9 years ago
Yes, we need a selftest that ensures all memory owned by CodeAlloc is readonly when we aren't in the middle of jitting.

Comment 28

9 years ago
verifying fixed by patches landing in all repos and acceptance suite passing. 

Leaving in-testsuite? until selftest media is generated

Updated

9 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.