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

Status: VERIFIED FIXED

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect, P2)

Description

[Steven Johnson]

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.

Comment 1

[Edwin Smith]

related:  Bug 506693

Comment 2

[Edwin Smith]

Attachment: 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.

Comment 3

[Edwin Smith]

Attachment: 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.

Comment 4

[Edwin Smith]

Attachment: 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.

Comment 5

[Rick Reitmaier]

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?

Comment 6

[Rick Reitmaier]

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

r+, assuming above comment is addressed

Comment 7

[Edwin Smith]

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

Comment 8

[Rick Reitmaier]

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.

Comment 9

[Edwin Smith]

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 10

[Nicholas Nethercote [inactive]]

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.

Comment 11

[Edwin Smith]

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

[Andreas Gal :gal]

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.

Comment 13

[Jeff Walden [:Waldo]]

"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.

Comment 14

[Edwin Smith]

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

Comment 15

[Edwin Smith]

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

Comment 16

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/69d5a3454a6e

Comment 17

[Nicholas Nethercote [inactive]]

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.

Comment 18

[Nicholas Nethercote [inactive]]

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.

Comment 19

[Edwin Smith]

Attachment: (do-over) Adds page protection SPI, and fixes bogus VMPI_setPageProtection calls from the first patch

Applies to nanojit-central tip

Comment 20

[Nicholas Nethercote [inactive]]

Backed out (with Graydon's help) from TM:

http://hg.mozilla.org/tracemonkey/rev/d85d14f9ff3d

Comment 21

[Nicholas Nethercote [inactive]]

Patch looks good, but I'll run it on the TM try server before giving r+ :)

Comment 22

[Nicholas Nethercote [inactive]]

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.

Comment 23

[Edwin Smith]

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

Comment 24

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/29fc29e37fe7

Comment 25

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/69d5a3454a6e

Comment 26

[Brent Baker]

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

Comment 27

[Edwin Smith]

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

[Brent Baker]

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

Leaving in-testsuite? until selftest media is generated