<HTML><BODY><div>Hi, Igor!</div><div>Thanks for the patch and the fixes!</div><div>LGTM</div><div> </div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div></div><div> </div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Вторник, 5 сентября 2023, 14:49 +03:00 от Igor Munkin <imun@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16939145520885013705_BODY">Sergey,<br><br>Thanks for your review! I've replied to your comments inline.<br><br>On 03.09.23, Sergey Kaplun wrote:<br>> Hi, Igor!<br>> Thanks for the patch!<br>> Please, consider my comments below.<br>><br>> On 01.09.23, Igor Munkin wrote:<br>> > From: Mike Pall <mike><br>> ><br>> > Reported by Sergey Kaplun.<br>> ><br>> > (cherry picked from commit c50232eb320d56d526ba5e6cb5bda8cf5a848a55)<br>> ><br>> > Unfortunately, <exit> call had been missing for a long time for the case<br>> > when <mprotect> fails within <mcode_protect>. Though the patch per se is<br>> > quite trivial, the test is not at all. It exploits the fact, that<br>><br>> Typo: s/fact,/fact/<br>><br>> > <mprotect> is used only for protecting area for mcode or callback<br>><br>> Typo: s/area/the area/<br>><br>> > function pointers. Hence, if the test doesn't use FFI at all, it is<br>> > guaranteed that the only <mprotect> called in LuaJIT runtime locates in<br>> > <mcode_protect> (that is not true for Tarantool, so the test is disabled<br>> > for integration testing routine). Furthermore, overloading <mprotect> on<br>><br>> Typo: s/for/for the/<br>><br>> > macOS occurs to be not an easy ride either, so running the test on macOS<br>><br>> s/occurs to be/is/<br>> Feel free to ignore.<br>><br>> > is disabled, since this is the common part for all platforms and<br>><br>> Typo: s/disabled,/disabled/<br>><br>> > everything can be checked on Linux in a much more easier way.<br>><br>> Typo: s/more//<br>><br>> ><br><br>Thanks for proofreading the commit message; the new one with the fixes<br>can be found below:<br><br>================================================================================<br><br>Always exit after machine code page protection change fails.<br><br>Reported by Sergey Kaplun.<br><br>(cherry picked from commit c50232eb320d56d526ba5e6cb5bda8cf5a848a55)<br><br>Unfortunately, <exit> call had been missing for a long time for the case<br>when <mprotect> fails within <mcode_protect>. Though the patch per se is<br>quite trivial, the test is not at all. It exploits the fact that<br><mprotect> is used only for protecting the area for mcode or callback<br>function pointers. Hence, if the test doesn't use FFI at all, it is<br>guaranteed that the only <mprotect> called in LuaJIT runtime locates in<br><mcode_protect> (that is not true for Tarantool, so the test is disabled<br>for the integration testing routine). Furthermore, attempts to overload<br><mprotect> on macOS occur to be not an easy ride either, so running the<br>test on macOS is disabled since this is the common part for all<br>platforms and everything can be checked on Linux in a much easier way.<br><br>Igor Munkin:<br>* added the description and the test for the problem<br><br>Part of tarantool/tarantool#8825<br><br>================================================================================<br><br>> > Igor Munkin:<br>> > * added the description and the test for the problem<br>> ><br>> > Part of tarantool/tarantool#8825<br>> ><br>> > Signed-off-by: Igor Munkin <<a href="/compose?To=imun@tarantool.org">imun@tarantool.org</a>><br>> > ---<br>> ><br>> > Branch: <a href="https://github.com/tarantool/luajit/tree/imun/lj-802-panic-at-mcode-protfail" target="_blank">https://github.com/tarantool/luajit/tree/imun/lj-802-panic-at-mcode-protfail</a><br>> > Tarantool PR: <a href="https://github.com/tarantool/tarantool/pull/9077" target="_blank">https://github.com/tarantool/tarantool/pull/9077</a><br>> > Related issues:<br>> > * <a href="https://github.com/tarantool/tarantool/issues/8825" target="_blank">https://github.com/tarantool/tarantool/issues/8825</a><br>> > * <a href="https://github.com/LuaJIT/LuaJIT/issues/802" target="_blank">https://github.com/LuaJIT/LuaJIT/issues/802</a><br>> ><br><br><snipped><br><br>> > diff --git a/test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua b/test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua<br>> > new file mode 100644<br>> > index 00000000..83a9ae2e<br>> > --- /dev/null<br>> > +++ b/test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua<br>><br>> The test doesn't fail before the patch<br>><br>> Neither if run it from command line:<br>><br>> | LUA_PATH="src/?.lua;test/tarantool-tests/?.lua;test/tarantool-tests/?/init.lua;" LD_LIBRARY_PATH="test/tarantool-tests/lj-802-panic-at-mcode-protfail/" src/luajit test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua<br>> | TAP version 13<br>> | 1..3<br>> | ok - Panic occurred as a result of <mprotect> failure<br>> | ok - LuaJIT exited as a result of the panic (error check)<br>> | ok - LuaJIT exited as a result of the panic (poison check)<br>><br>> nor as as part of `make tarantool-tests` (checked with or wo GC64).<br>><br>> Compiled as the following:<br>> | cmake . -DLUAJIT_ENABLE_WARNINGS=OFF -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_APICHECK=ON -DLUA_USE_ASSERT=ON -DLUAJIT_ENABLE_GC64=ON && make -j<br>><br>> Tested with the following diff:<br>> ===================================================================<br>> diff --git a/src/lj_mcode.c b/src/lj_mcode.c<br>> index a88d16bd..9b59053a 100644<br>> --- a/src/lj_mcode.c<br>> +++ b/src/lj_mcode.c<br>> @@ -180,7 +180,7 @@ static void mcode_protect(jit_State *J, int prot)<br>> #define MCPROT_RUN MCPROT_RX<br>><br>> /* Protection twiddling failed. Probably due to kernel security. */<br>> -static LJ_NORET LJ_NOINLINE void mcode_protfail(jit_State *J)<br>> +static LJ_NOINLINE void mcode_protfail(jit_State *J)<br>> {<br>> lua_CFunction panic = J2G(J)->panic;<br>> if (panic) {<br>> @@ -188,7 +188,7 @@ static LJ_NORET LJ_NOINLINE void mcode_protfail(jit_State *J)<br>> setstrV(L, L->top++, lj_err_str(L, LJ_ERR_JITPROT));<br>> panic(L);<br>> }<br>> - exit(EXIT_FAILURE);<br>> + // exit(EXIT_FAILURE);<br>> }<br>><br>> /* Change protection of MCode area. */<br>> ===================================================================<br>><br>> WDIDW?<br><br>Building LuaJIT with the internal assertions, LOL. Anyway, thanks for<br>noticing this. I've added additional check to the test (see the<br>incremental diff below):<br><br>================================================================================<br><br>diff --git a/test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua b/test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua<br>index 83a9ae2e..f4dc4e1c 100644<br>--- a/test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua<br>+++ b/test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua<br>@@ -19,7 +19,7 @@ local test = tap.test('lj-flush-on-trace'):skipcond({<br> ['<mprotect> cannot be overridden on macOS'] = jit.os == 'OSX',<br> })<br> <br>-test:plan(3)<br>+test:plan(4)<br> <br> -- <makecmd> runs %testname%/script.lua by <LUAJIT_TEST_BINARY><br> -- with the given environment, launch options and CLI arguments.<br>@@ -35,6 +35,8 @@ test:like(output, 'runtime code generation failed, restricted kernel%?',<br> 'Panic occurred as a result of <mprotect> failure')<br> test:unlike(output, 'Segmentation fault',<br> 'LuaJIT exited as a result of the panic (error check)')<br>+test:unlike(output, 'Aborted',<br>+ 'LuaJIT exited as a result of the panic (assertion check)')<br> test:unlike(output, poison,<br> 'LuaJIT exited as a result of the panic (poison check)')<br> <br><br>================================================================================<br><br>><br>> > @@ -0,0 +1,41 @@<br>> > +local tap = require('tap')<br>> > +local test = tap.test('lj-flush-on-trace'):skipcond({<br>> > + ['Test requires JIT enabled'] = not jit.status(),<br>> > + ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',<br>> > + -- XXX: This test has to check the particular patch for<br>> > + -- <mcode_protfail> and <mprotect> is overloaded for this<br>><br>> Typo: s/<mcode_protfail>/<mcode_protfail>,/<br>><br>> > + -- purpose. However, <mprotect> is used widely in Tarantool<br>><br>> Typo: s/is used widely/is widely used/<br>><br>> > + -- to play with fiber stacks, so overriding <mprotect> is not<br>> > + -- suitable to test this feature in Tarantool.<br>> > + -- luacheck: no global<br>> > + ['<mprotect> overriding can break Tarantool'] = _TARANTOOL,<br>> > + -- XXX: Unfortunately, it's too hard to overload (or even<br>> > + -- impossible, who knows, since Cupertino fellows do not<br>> > + -- provide any information about their system) something from<br>> > + -- libsystem_kernel.dylib (the library providing <mprotect>).<br>> > + -- All in all, this test checks the part, that is common for all<br>><br>> Typo: s/part,/part/<br>><br>> > + -- platforms, so it's not vital to run this test on macOS, since<br>><br>> Typo: s/macOS,/macOS/<br>><br>> > + -- everything can be checked on Linux in a much more easier way.<br>><br>> Typo: s/more//<br>><br>> > + ['<mprotect> cannot be overridden on macOS'] = jit.os == 'OSX',<br>> > +})<br>> > +<br>> > +test:plan(3)<br>> > +<br><br>All the typos in the test code are fixed; the diff is below:<br><br>================================================================================<br><br>diff --git a/test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua b/test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua<br>index f4dc4e1c..94f4314f 100644<br>--- a/test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua<br>+++ b/test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua<br>@@ -3,8 +3,8 @@ local test = tap.test('lj-flush-on-trace'):skipcond({<br> ['Test requires JIT enabled'] = not jit.status(),<br> ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',<br> -- XXX: This test has to check the particular patch for<br>- -- <mcode_protfail> and <mprotect> is overloaded for this<br>- -- purpose. However, <mprotect> is used widely in Tarantool<br>+ -- <mcode_protfail>, and <mprotect> is overloaded for this<br>+ -- purpose. However, <mprotect> is widely used in Tarantool<br> -- to play with fiber stacks, so overriding <mprotect> is not<br> -- suitable to test this feature in Tarantool.<br> -- luacheck: no global<br>@@ -13,9 +13,9 @@ local test = tap.test('lj-flush-on-trace'):skipcond({<br> -- impossible, who knows, since Cupertino fellows do not<br> -- provide any information about their system) something from<br> -- libsystem_kernel.dylib (the library providing <mprotect>).<br>- -- All in all, this test checks the part, that is common for all<br>- -- platforms, so it's not vital to run this test on macOS, since<br>- -- everything can be checked on Linux in a much more easier way.<br>+ -- All in all, this test checks the part that is common for all<br>+ -- platforms, so it's not vital to run this test on macOS since<br>+ -- everything can be checked on Linux in a much easier way.<br> ['<mprotect> cannot be overridden on macOS'] = jit.os == 'OSX',<br> })<br> <br><br>================================================================================<br><br>> > diff --git a/test/tarantool-tests/lj-802-panic-at-mcode-protfail/script.lua b/test/tarantool-tests/lj-802-panic-at-mcode-protfail/script.lua<br>> > new file mode 100644<br>> > index 00000000..661099fa<br>> > --- /dev/null<br>> > +++ b/test/tarantool-tests/lj-802-panic-at-mcode-protfail/script.lua<br>> > @@ -0,0 +1,12 @@<br>> > +jit.opt.start('hotloop=1')<br>> > +<br>> > +-- Run a simple loop that triggers <mprotect> on trace assembling.<br>> > +local a = 0<br>> > +for i = 1, 3 do<br>><br>> Minor: Should it be 4 here to actually *run* a loop and be sure that we<br>> don't execute this part of the code?<br><br>But 3 is this dedicated iteration. Here are some details regarding the<br>loop recording:<br>* The first iteration is executed until the corresponding loop bytecode<br> (BC_FORL, IIRC) is reached.<br>* Starting from the first occurrence of the aforementioned bytecode, the<br> compiler starts recording the loop. This is the second iteration.<br>* To successfully finalize the compilation of the loop, the jump at that<br> bytecode should be done back to the beginning of the loop body (this<br> is the heuristic of JIT compiler, since the loop is not considered<br> "hot" otherwise and there is no reason to finalize this trace). If the<br> jump target is valid in the sense described above, the trace is<br> finalized and, since the jump targets back to the loop body, the next<br> loop iteration is run.<br>* Since the trace is already compiled, the third and the last iteration<br> is run via the mcode instead of VM interpreting the bytecode. At this<br> point the execution of the trace is finished with the corresponding<br> guard checking the "iterator" value.<br><br>So, if you want one "full" iteration and one "to-be-exited" iteration, I<br>can increment the right boundary of the loop, but I doubt that it's<br>strongly required here (since we check that the compilation is failed at<br>the assembling phase).<br><br>><br>> > + a = a + i<br>> > +end<br>> > +<br>> > +-- XXX: Just a simple contract output in case neither panic at<br>><br>> Typo: s/panic/the panic/<br>><br><br>All the typos in the script code are also fixed; the diff is below:<br><br>================================================================================<br><br>diff --git a/test/tarantool-tests/lj-802-panic-at-mcode-protfail/script.lua b/test/tarantool-tests/lj-802-panic-at-mcode-protfail/script.lua<br>index 661099fa..201a8ff2 100644<br>--- a/test/tarantool-tests/lj-802-panic-at-mcode-protfail/script.lua<br>+++ b/test/tarantool-tests/lj-802-panic-at-mcode-protfail/script.lua<br>@@ -6,7 +6,7 @@ for i = 1, 3 do<br> a = a + i<br> end<br> <br>--- XXX: Just a simple contract output in case neither panic at<br>+-- XXX: Just a simple contract output in case neither the panic at<br> -- <mprotect>, nor crash occurs (see for LUAJIT_UNPROTECT_MCODE in<br> -- lj_mcode.c for more info).<br> io.write(arg[1])<br><br>================================================================================<br><br>> > +-- <mprotect>, nor crash occurs (see for LUAJIT_UNPROTECT_MCODE in<br>> > +-- lj_mcode.c for more info).<br>> > +io.write(arg[1])<br>> > --<br>> > 2.30.2<br>> ><br>><br>> --<br>> Best regards,<br>> Sergey Kaplun<br><br>--<br>Best regards,<br>IM</div></div></div></div></blockquote><div> </div></BODY></HTML>