ext/opcache JIT: attempt to fix EPERM on unprotecting dasm buffer. by devnexen · Pull Request #13351 · php/php-src

devnexen · 2024-02-07T21:33:10Z

despite disabling protection before mprotect call, we still get an EPERM with ZTS build we set all PROT*flags. removing typo in mapped segments (flag in the wrong place even tough the os most likely discard it).

dstogov · 2024-02-08T03:59:44Z

I don't see how this affects MACOS/ZTS builds...
Please add MACOS_ARM64_DEBUG_ZTS and MACOS_X64_DEBUG_ZTS test jobs.

devnexen · 2024-02-08T08:56:37Z

cc @iluuu1994 when you have a moment, to double check if I ve done anything wrong in the CI's side. Thx.

iluuu1994 · 2024-02-08T10:55:16Z

@devnexen You can see the error here: https://github.com/php/php-src/actions/runs/7826796920

The workflow is not valid. .github/workflows/push.yml (Line: 131, Col: 16): Unrecognized named-value: 'max'. Located at position 1 within expression: max.debug && 'DEBUG' || 'RELEASE'

max should be matrix

devnexen · 2024-02-08T13:28:29Z

@devnexen You can see the error here: https://github.com/php/php-src/actions/runs/7826796920

The workflow is not valid. .github/workflows/push.yml (Line: 131, Col: 16): Unrecognized named-value: 'max'. Located at position 1 within expression: max.debug && 'DEBUG' || 'RELEASE'

max should be matrix

Now, it seems arm64 picks up x86 setting (bison path). No idea yet maybe the job hash ? I ll look later ..

iluuu1994 · 2024-02-08T13:33:28Z

@devnexen See https://github.com/php/php-src/pull/13299/files#diff-5634435a3b24e8a6d21e34340762047e16266acbedc19ecf829b9754de588d89, you didn't copy all changes from GH-13299.

devnexen · 2024-02-09T20:59:22Z

@devnexen See https://github.com/php/php-src/pull/13299/files#diff-5634435a3b24e8a6d21e34340762047e16266acbedc19ecf829b9754de588d89, you didn't copy all changes from GH-13299.
Thanks. Let me know if I did a mess in your department :)

iluuu1994 · 2024-02-09T21:41:13Z

@devnexen If I may ask, do we need to backport all of these changes? I was under the impression you merely wanted to test this change for 8.2. For warnings, you could have temporarily disabled --enable-werror. As this shows the build to be correct, I'd prefer just merging the functional changes for macOS. If we want to backport CI, we should do so in a separate PR anyway.

devnexen · 2024-02-09T21:52:52Z

@devnexen If I may ask, do we need to backport all of these changes? I was under the impression you merely wanted to test this change for 8.2. For warnings, you could have temporarily disabled --enable-werror. As this shows the build to be correct, I'd prefer just merging the functional changes for macOS. If we want to backport CI, we should do so in a separate PR anyway.

Not necessarily I kept as separated commits on purpose to see which worths what. Ok for the CI change, at least we see the original change fix the issue on macos arm64.

iluuu1994 · 2024-02-09T23:21:52Z

@devnexen Great! I don't mind back-porting CI. nightly.yml can be dropped, it has no effect on older branches.

dstogov · 2024-02-12T05:57:54Z

I understood, this PR attempts to fix MACOS-ARM64-ZTS builds.
According to the source changes in ext/opcache/jit/zend_jit.c and ext/opcache/shared_alloc_mmap.c, this also affects MACOS-X64-ZTS builds, that worked fine.
I afraid, this patch completely removes WRITE protection of the JIT memory segment on MACOS-X64-ZTS.
Please confirm (I may be wrong).
I think, removing this protection in the old PHP releases is a completely unacceptable solution.

Also the PR needs to be cleaned up. It touches many unrelated files. I suspect. this is because of change of the base branch.

dstogov · 2024-02-12T06:00:45Z

Do PHP-8.2/3-MACOS-ARM64-ZTS builds also fail? (they are not covered by "nightly" workflow).

devnexen · 2024-02-12T07:53:41Z

I understood, this PR attempts to fix MACOS-ARM64-ZTS builds. According to the source changes in ext/opcache/jit/zend_jit.c and ext/opcache/shared_alloc_mmap.c, this also affects MACOS-X64-ZTS builds, that worked fine. I afraid, this patch completely removes WRITE protection of the JIT memory segment on MACOS-X64-ZTS. Please confirm (I may be wrong). I think, removing this protection in the old PHP releases is a completely unacceptable solution.

The change done in ext/opcache/shared_alloc_mmap.c is a correct one though, MAP_JIT not only is. no-op on x86 but also should not be in the flags variable in the first place (since it contains PROT_* flags).

However the other change should had been done only for arm64. I ll go back to it later.

Also the PR needs to be cleaned up. It touches many unrelated files. I suspect. this is because of change of the base branch.

I did not plan to push other changes, was just to display more data on CI.

dstogov · 2024-02-12T09:34:34Z

The change done in ext/opcache/shared_alloc_mmap.c is a correct one though, MAP_JIT not only is. no-op on x86 but also should not be in the flags variable in the first place (since it contains PROT_* flags).

It seems like this is the real mistake. MAP_JIT should be added to mmap() flags instead of prot argument.
I'll try to check this.

dstogov · 2024-02-12T15:43:02Z

ext/opcache/jit/zend_jit.c

+#if defined(ZTS) && (!defined(__APPLE__) || !defined(__aarch64__))
 		opts |= PROT_EXEC;
 #endif


This disables EXEC permissions for all threads. Therefore other threads are going to crash while this one won't return the permissions back.

According to https://developer.apple.com/documentation/apple-silicon/porting-just-in-time-compilers-to-apple-silicon we should use pthread_jit_write_protect_np() without mprotect(), but I wasn't able to make it work.
In my opinion the code should be changed in this way https://github.com/php/php-src/pull/13375/files

It seems pthread_jit_write_protect_supported_np() somehow returns false (on github actions).
May be we should set com.apple.security.cs.allow-jit entitlement...
I don't have access to Mac and I have no idea how to do this.

I think it needs codesign (and probably a proper apple license).

I think it needs codesign (and probably a proper apple license).

Can you try this?

using self-signed identity.

will give it a try sometime today definitively.

See discussion in php#13351. Closes phpGH-13396

Apple Silicon has stricter rules about rwx mmap regions. They need to be created using the MAP_JIT flag. However, the MAP_JIT seems to be incompatible with MAP_SHARED. ZTS requires MAP_SHARED so that some threads may execute code from a page while another writes/appends to it. We did not find another solution, other than completely disabling JIT for Apple Silicon + ZTS. See discussion in #13351. Co-authored-by: Peter Kokot <[email protected]> Fixes GH-13400 Closes GH-13396

ext/opcache JIT: attempt to fix EPERM on unprotecting dasm buffer.

39a7d49

despite disabling protection before mprotect call, we still get an EPERM with ZTS build we set all PROT*flags. removing typo in mapped segments (flag in the wrong place even tough the os most likely discard it).

github-actions bot added the Extension: opcache label Feb 7, 2024

devnexen changed the base branch from master to PHP-8.2 February 7, 2024 21:33

devnexen marked this pull request as ready for review February 7, 2024 22:21

devnexen requested a review from dstogov February 7, 2024 22:21

devnexen force-pushed the fix_macos_arm64_jit_eperm_mprotect branch 3 times, most recently from f945d93 to 0214a22 Compare February 8, 2024 08:26

devnexen force-pushed the fix_macos_arm64_jit_eperm_mprotect branch 4 times, most recently from 92ba014 to b6fc71e Compare February 8, 2024 13:16

adding two more macos jobs

872b79d

devnexen force-pushed the fix_macos_arm64_jit_eperm_mprotect branch from b6fc71e to 872b79d Compare February 8, 2024 18:48

fix openssl call signature

1889d15

github-actions bot added Extension: openssl Extension: bcmath labels Feb 8, 2024

bcmath fixing wrong warning due to likely old fashion C

91a13de

devnexen force-pushed the fix_macos_arm64_jit_eperm_mprotect branch from f0aa9a0 to 91a13de Compare February 8, 2024 20:00

libcdb build fix

81e2e1d

github-actions bot added the Extension: dba label Feb 8, 2024

devnexen added 3 commits February 8, 2024 20:17

libinifile build fix

0bd2954

libflatfile build fix

0b6b89b

exif build fix

e4864d5

github-actions bot added the Extension: exif label Feb 8, 2024

github-actions bot added the Category: Optimizer label Feb 8, 2024

mysqli unused

05d3e31

github-actions bot added the Extension: mysqli label Feb 8, 2024

soap build fix

6782262

github-actions bot added the Extension: soap label Feb 8, 2024

devnexen added 2 commits February 8, 2024 21:52

backport crc32 mac fix

59a9c8a

mysqlnd build fix

2d45d6f

github-actions bot added the Extension: mysqlnd label Feb 8, 2024

fpm build fix

e8f8372

github-actions bot added the SAPI: fpm label Feb 8, 2024

further CI update

083ff0a

devnexen force-pushed the fix_macos_arm64_jit_eperm_mprotect branch from 05b164f to 083ff0a Compare February 8, 2024 23:26

fix ccache key

d5f8f1c

devnexen force-pushed the fix_macos_arm64_jit_eperm_mprotect branch from df8a1f6 to d5f8f1c Compare February 8, 2024 23:57

ignore this

374912d

dstogov reviewed Feb 12, 2024

View reviewed changes

devnexen closed this Feb 13, 2024

iluuu1994 mentioned this pull request Feb 15, 2024

Disable JIT on Apple Silicon + ZTS #13396

Closed

iluuu1994 added a commit to iluuu1994/php-src that referenced this pull request Feb 15, 2024

Disable JIT on Apple Silicon + ZTS

637f1cc

See discussion in php#13351. Closes phpGH-13396

iluuu1994 added a commit to iluuu1994/php-src that referenced this pull request Feb 15, 2024

Disable JIT on Apple Silicon + ZTS

881fe21

See discussion in php#13351. Closes phpGH-13396

Conversation

devnexen commented Feb 7, 2024

Uh oh!

dstogov commented Feb 8, 2024

Uh oh!

devnexen commented Feb 8, 2024

Uh oh!

iluuu1994 commented Feb 8, 2024

Uh oh!

devnexen commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iluuu1994 commented Feb 8, 2024

Uh oh!

devnexen commented Feb 9, 2024

Uh oh!

iluuu1994 commented Feb 9, 2024

Uh oh!

devnexen commented Feb 9, 2024

Uh oh!

iluuu1994 commented Feb 9, 2024

Uh oh!

dstogov commented Feb 12, 2024

Uh oh!

dstogov commented Feb 12, 2024

Uh oh!

devnexen commented Feb 12, 2024

Uh oh!

dstogov commented Feb 12, 2024

Uh oh!

dstogov Feb 12, 2024

Choose a reason for hiding this comment

Uh oh!

devnexen Feb 12, 2024

Choose a reason for hiding this comment

Uh oh!

dstogov Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

dstogov Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

devnexen Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

devnexen commented Feb 8, 2024 •

edited

Loading