Re: [Tarantool-patches] [PATCH luajit] Always exit after machine code page protection change fails.

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 13460 bytes --]


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

[-- Attachment #2: Type: text/html, Size: 16442 bytes --]