[Tarantool-patches] [PATCH luajit 1/2] Add support for full-range 64 bit lightuserdata.
Igor Munkin
imun at tarantool.org
Tue Jul 27 16:59:41 MSK 2021
Sergey,
Thanks for the patch! Please consider my comments below.
On 06.07.21, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> (cherry picked from commit e9af1abec542e6f9851ff2368e7f196b6382a44c)
>
> LuaJIT uses special NaN-tagging technique to store internal type on
> the Lua stack. In case LJ_GC64 first 13 bits are set in special NaN
> type (0xfff8...). FPU generates the only one type. The next 4 bits are
> used for an internal LuaJIT type of object on stack. The next 47 bits
> are used for storing this object's content. For userdata, it is its
> address. In case arm64 the pointer can have more than 47 significant
> bits [1]. In this case the error BADLU error is raised.
This is not right for (LJ_64 && !LJ_GC64) case, but AFAIU you are
writing about LJ_GC64 here since this is the only option for ARM64.
>
> For the support of full 64-bit range lightuserdata pointers two new
> fields in GCState are added:
>
> `lightudseg` - vector of segments of lightuserdata storing 32-bit
I don't get whether vector or segments or lightuserdata store these
32-bit values :)
> values. MS 25 bits equal to MS bits of lightuserdata address, the
It's better to use either "MSB" acronym or "most significant 25 bits".
> rest are filled with zeros. The lentgh of the vector is power of 2.
>
> `lightudnum` - the length - 1 of aforementioned vector (up to 255).
>
> When lightuserdata is pushed on the stack, if its segment is not stored
> in vector new value is pushed on top. The maximum amount of segments is
On top of the stack or <lightudseg>? The wording is ambiguous. AFAIU,
you're writing about the vector, so the preferable verb is "append" or
"push back". Or simply "add", since nobody cares about the location of
the particular key.
> 256, BADLU error is raised in case when user tried to add userdata with
> the new 257-th segment.
It's worth to mention that such hack with segments still doesn't cover
the full VA space, but only those areas covered with the segments. It
was unclear to me at first.
>
> Also, in this patch all internal usage of lightuserdata (for hooks,
> profilers, built-in package, ir and so on) is changed to special values
Typo: s/ir/IR/ since this is an acronym.
> on Lua Stack.
>
> Also, conversion of TValue to ffi C type with store is no longer
Ditto for FFI.
> compiled for lightuserdata.
>
> [1]: https://www.kernel.org/doc/html/latest/arm64/memory.html
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Needed for tarantool/tarantool#6154
> Resolves tarantool/tarantool#2712
Please move "Resolves" tag on top. BTW, it's rather "Fixes", isn't it?
> ---
> doc/status.html | 11 ----
> src/jit/dump.lua | 4 +-
> src/lib_debug.c | 12 ++--
> src/lib_jit.c | 14 ++---
> src/lib_package.c | 8 +--
> src/lib_string.c | 2 +-
> src/lj_api.c | 40 +++++++++++--
> src/lj_ccall.c | 2 +-
> src/lj_cconv.c | 2 +-
> src/lj_crecord.c | 6 +-
> src/lj_dispatch.c | 2 +-
> src/lj_ir.c | 6 +-
> src/lj_obj.c | 5 +-
> src/lj_obj.h | 57 ++++++++++++-------
> src/lj_snap.c | 7 ++-
> src/lj_state.c | 6 ++
> src/lj_strfmt.c | 2 +-
> test/tarantool-tests/CMakeLists.txt | 1 +
> .../lj-49-bad-lightuserdata.test.lua | 10 ++++
> .../lj-49-bad-lightuserdata/CMakeLists.txt | 1 +
> .../testlightuserdata.c | 52 +++++++++++++++++
> 21 files changed, 183 insertions(+), 67 deletions(-)
> create mode 100644 test/tarantool-tests/lj-49-bad-lightuserdata.test.lua
> create mode 100644 test/tarantool-tests/lj-49-bad-lightuserdata/CMakeLists.txt
> create mode 100644 test/tarantool-tests/lj-49-bad-lightuserdata/testlightuserdata.c
>
<snipped>
> diff --git a/src/lib_debug.c b/src/lib_debug.c
> index f112b5bc..8fdfda03 100644
> --- a/src/lib_debug.c
> +++ b/src/lib_debug.c
<snipped>
> @@ -283,13 +283,13 @@ LJLIB_CF(debug_setuservalue)
>
> /* ------------------------------------------------------------------------ */
>
> -#define KEY_HOOK ((void *)0x3004)
> +#define KEY_HOOK (U64x(80000000,00000000)|'h')
>
> static void hookf(lua_State *L, lua_Debug *ar)
> {
> static const char *const hooknames[] =
> {"call", "return", "line", "count", "tail return"};
> - lua_pushlightuserdata(L, KEY_HOOK);
> + (L->top++)->u64 = KEY_HOOK;
Side note: Comment desired. I see no guarantees there was no stack
reallocations for the removed <lua_pushlightuserdata>. Fortunately,
there are 5 more slots in the "red zone" for metamethods, and the stack
will be resized within "if" block if needed, but I don't feel safe in
case there is no hook function in registry.
I leave this on you whether to add the comment for this or not.
> lua_rawget(L, LUA_REGISTRYINDEX);
> if (lua_isfunction(L, -1)) {
> lua_pushstring(L, hooknames[(int)ar->event]);
> @@ -334,7 +334,7 @@ LJLIB_CF(debug_sethook)
> count = luaL_optint(L, arg+3, 0);
> func = hookf; mask = makemask(smask, count);
> }
> - lua_pushlightuserdata(L, KEY_HOOK);
Side note: Comment desired. I see no guarantees there was no stack
reallocations for the removed <lua_pushlightuserdata>. Fortunately,
there are 5 more slots in the "red zone" for metamethods, so we are safe
and the reallocation occurs in <lua_pushvalue> below if one needed.
Anyway, such hacks are fragile, IMHO.
I leave this on you whether to add the comment for this or not.
> + (L->top++)->u64 = KEY_HOOK;
> lua_pushvalue(L, arg+1);
> lua_rawset(L, LUA_REGISTRYINDEX);
> lua_sethook(L, func, mask, count);
> @@ -349,7 +349,7 @@ LJLIB_CF(debug_gethook)
> if (hook != NULL && hook != hookf) { /* external hook? */
> lua_pushliteral(L, "external hook");
> } else {
> - lua_pushlightuserdata(L, KEY_HOOK);
> + (L->top++)->u64 = KEY_HOOK;
Ditto.
> lua_rawget(L, LUA_REGISTRYINDEX); /* get hook */
> }
> lua_pushstring(L, unmakemask(mask, buff));
<snipped>
> diff --git a/src/lib_package.c b/src/lib_package.c
> index a0bca572..8573b9f9 100644
> --- a/src/lib_package.c
> +++ b/src/lib_package.c
<snipped>
> @@ -445,14 +445,14 @@ static int lj_cf_package_require(lua_State *L)
> else
> lua_pop(L, 1);
> }
> - lua_pushlightuserdata(L, sentinel);
> + (L->top++)->u64 = KEY_SENTINEL;
Ditto.
> lua_setfield(L, 2, name); /* _LOADED[name] = sentinel */
> lua_pushstring(L, name); /* pass name as argument to module */
> lua_call(L, 1, 1); /* run loaded module */
> if (!lua_isnil(L, -1)) /* non-nil return? */
> lua_setfield(L, 2, name); /* _LOADED[name] = returned value */
> lua_getfield(L, 2, name);
<snipped>
> diff --git a/src/lj_api.c b/src/lj_api.c
> index c9b5d220..c7a0b327 100644
> --- a/src/lj_api.c
> +++ b/src/lj_api.c
<snipped>
> diff --git a/src/lj_ccall.c b/src/lj_ccall.c
> index 06d1b3bd..f26f1fea 100644
> --- a/src/lj_ccall.c
> +++ b/src/lj_ccall.c
> @@ -1150,7 +1150,7 @@ int lj_ccall_func(lua_State *L, GCcdata *cd)
> lj_vm_ffi_call(&cc);
> if (cts->cb.slot != ~0u) { /* Blacklist function that called a callback. */
> TValue tv;
> - setlightudV(&tv, (void *)cc.func);
> + tv.u64 = ((uintptr_t)(void *)cc.func >> 2) | U64x(800000000, 00000000);
Side note: Comment desired. The reason why this hack works is that
compilers align the functions up to the machine word (i.e. 8 bytes),
hence two LSB are *likely* be zeros. Furthermore, functions calling the
callbacks are *likely* contains of more than 8 bytes of instructions.
However, I wonder whether we can create two hand-crafted assembler
functions, that are placed one right after another within 8 bytes, so
blacklisting one of them affects another one. I believe this need to be
checked (aside of this patchset) and reported to the upstream if the
issue occurs.
I leave this on you whether to add the comment for this or not.
> setboolV(lj_tab_set(L, cts->miscmap, &tv), 1);
> }
> ct = (CType *)((intptr_t)ct+(intptr_t)cts->tab); /* May be reallocated. */
<snipped>
> diff --git a/src/lj_crecord.c b/src/lj_crecord.c
> index e32ae23e..59186dab 100644
> --- a/src/lj_crecord.c
> +++ b/src/lj_crecord.c
<snipped>
> @@ -1209,8 +1208,7 @@ static int crec_call(jit_State *J, RecordFFData *rd, GCcdata *cd)
> TRef tr;
> TValue tv;
> /* Check for blacklisted C functions that might call a callback. */
> - setlightudV(&tv,
> - cdata_getptr(cdataptr(cd), (LJ_64 && tp == IRT_P64) ? 8 : 4));
> + tv.u64 = ((uintptr_t)cdata_getptr(cdataptr(cd), (LJ_64 && tp == IRT_P64) ? 8 : 4) >> 2) | U64x(800000000, 00000000);
Returning to the previous comment, looks like you can just turn JIT off
for the one handcrafted function to implicitly blacklist another one.
> if (tvistrue(lj_tab_get(J->L, cts->miscmap, &tv)))
> lj_trace_err(J, LJ_TRERR_BLACKL);
> if (ctype_isvoid(ctr->info)) {
<snipped>
> diff --git a/src/lj_ir.c b/src/lj_ir.c
> index 5baece67..2f7ddb24 100644
> --- a/src/lj_ir.c
> +++ b/src/lj_ir.c
> @@ -386,8 +386,10 @@ void lj_ir_kvalue(lua_State *L, TValue *tv, const IRIns *ir)
> case IR_KPRI: setpriV(tv, irt_toitype(ir->t)); break;
> case IR_KINT: setintV(tv, ir->i); break;
> case IR_KGC: setgcV(L, tv, ir_kgc(ir), irt_toitype(ir->t)); break;
> - case IR_KPTR: case IR_KKPTR: setlightudV(tv, ir_kptr(ir)); break;
> - case IR_KNULL: setlightudV(tv, NULL); break;
> + case IR_KPTR: case IR_KKPTR:
> + setnumV(tv, (lua_Number)(uintptr_t)ir_kptr(ir));
[Looking forward to the day this will crash for 53-bit VA addresses]
> + break;
> + case IR_KNULL: setintV(tv, 0); break;
> case IR_KNUM: setnumV(tv, ir_knum(ir)->n); break;
> #if LJ_HASFFI
> case IR_KINT64: {
<snipped>
> diff --git a/src/lj_snap.c b/src/lj_snap.c
> index 4cf27e76..2f7cf80a 100644
> --- a/src/lj_snap.c
> +++ b/src/lj_snap.c
> @@ -626,7 +626,12 @@ static void snap_restoreval(jit_State *J, GCtrace *T, ExitState *ex,
> IRType1 t = ir->t;
> RegSP rs = ir->prev;
> if (irref_isk(ref)) { /* Restore constant slot. */
> - lj_ir_kvalue(J->L, o, ir);
> + if (ir->o == IR_KPTR) {
> + o->u64 = (uint64_t)(uintptr_t)ir_kptr(ir);
Side note: It's strange, why Mike didn't make this trick within
<lj_ir_kvalue>, but decided to play with lua_Number *and* do not use
<lj_ir_kvalue> for IR_KPTR for this case. I haven't checked precisely
whether IR_KPTR is passed to <lj_ir_kvalue> anywhere in the JIT.
> + } else {
> + lua_assert(!(ir->o == IR_KKPTR || ir->o == IR_KNULL));
Side note: BTW, this choice is quite obvious, since both IR_KKPTR and
IR_KNULL are limited by LuaJIT defines.
> + lj_ir_kvalue(J->L, o, ir);
> + }
> return;
> }
> if (LJ_UNLIKELY(bloomtest(rfilt, ref)))
<snipped>
> diff --git a/test/tarantool-tests/lj-49-bad-lightuserdata/testlightuserdata.c b/test/tarantool-tests/lj-49-bad-lightuserdata/testlightuserdata.c
> new file mode 100644
> index 00000000..801c7fe1
> --- /dev/null
> +++ b/test/tarantool-tests/lj-49-bad-lightuserdata/testlightuserdata.c
> @@ -0,0 +1,52 @@
> +#include <lua.h>
> +#include <lauxlib.h>
> +
> +#include <sys/mman.h>
> +#include <unistd.h>
> +
> +#undef NDEBUG
> +#include <assert.h>
> +
> +#define START ((void *)-1)
> +
> +static int longptr(lua_State *L)
> +{
> + /*
> + * We know that for arm64 at least 48 bits are available.
> + * So emulate manually push of lightuseradata within
> + * this range.
> + */
> + void *longptr = (void *)(1llu << 48);
> + lua_pushlightuserdata(L, longptr);
> + assert(longptr == lua_topointer(L, -1));
> + /*
> + * If start mapping address is not NULL, then the kernel
> + * takes it as a hint about where to place the mapping, so
> + * we try to get the highest memory address by hint
> + * equals -1.
> + */
BTW, I guess it's more natural to split this function into two: one for
hand-crafted pointer, and another for one obtained by <mmap>.
> + const size_t pagesize = getpagesize();
Please declare the variables at the beginning of the scope.
> + void *mmaped = mmap(START, pagesize, PROT_NONE,
> + MAP_PRIVATE | MAP_ANON, -1, 0);
Typo: Odd arguments alignment.
> + if (mmaped != MAP_FAILED) {
> + lua_pushlightuserdata(L, mmaped);
> + assert(mmaped == lua_topointer(L, -1));
> + assert(munmap(mmaped, pagesize) == 0);
> + }
> + /* Clear our stack. */
> + lua_pop(L, 0);
> + lua_pushboolean(L, 1);
> + return 1;
> +}
> +
> +static const struct luaL_Reg testlightuserdata[] = {
> + {"longptr", longptr},
> + {NULL, NULL}
> +};
> +
> +LUA_API int luaopen_testlightuserdata(lua_State *L)
> +{
> + luaL_register(L, "testlightuserdata", testlightuserdata);
> + return 1;
> +}
> +
> --
> 2.31.0
>
--
Best regards,
IM
More information about the Tarantool-patches
mailing list