| Message ID | 20210827144942.55981-1-christian.storm@siemens.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Lua: Replace lightuserdata to fix LuaJIT on aarch64 with 48bit VA | expand |
Hi Christian, Michael, On 27.08.21 16:49, Christian Storm wrote: > Current aarch64 (distro) kernels ship with CONFIG_ARM64_VA_BITS_48 > enabled, see, e.g., [1] for Debian's kernel, resulting in a LuaJIT > panic "bad light userdata pointer" on aarch64 with distro-packaged > old(er) LuaJIT versions that limit lightuserdata to 47 bit pointers. > > One mitigation option is to use a kernel with less VA bits, ruling > out using distro kernels untouched. > Another mitigation is to use the latest LuaJIT as this issue has > been resolved, see [2,3], ruling out using the (older) distro's > LuaJIT package untouched -- until distros eventually catch up. > > Hence, to allow SWUpdate on systems with >47 bits VA to run on current > distros' kernel and LuaJIT packages, replace lightuserdata with "full" > userdata. For other systems with with <=47 bits VA, this change has no > effect other than Lua's gc having to clean up the "full" userdata of > sizeof(struct dict*) when it becomes unreferenced: > For sw-description embedded scripts, this is happens after having > interpreted sw-description. > For a Lua handler, this happens at its next call as an explicit free > and gc cycle call would occupy more permanent space. > Frankly speaking, this wants to fix an issue in another package into SWUpdate. The right fix here is not to change kernel, but as you propose to update LuaJIT that contains the fix. Anyway, this can be merged into SWUpdate if there are not any drawbacks. We have no specific performance issue to prefer lighuserdata (that is pointer) to generic userdata. > [1] https://salsa.debian.org/kernel-team/linux/-/blob/bullseye/debian/config/arm64/config#L13 > [2] https://github.com/LuaJIT/LuaJIT/issues/49 > [3] https://github.com/LuaJIT/LuaJIT/pull/230 > > Signed-off-by: Christian Storm <christian.storm@siemens.com> > Signed-off-by: Michael Adler <michael.adler@siemens.com> > --- > corelib/lua_interface.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c > index a6da6f2..e1a9c3e 100644 > --- a/corelib/lua_interface.c > +++ b/corelib/lua_interface.c > @@ -854,7 +854,7 @@ static int l_get_bootenv(lua_State *L) { > } > > static int l_set_bootenv(lua_State *L) { > - struct dict *bootenv = (struct dict *)lua_touserdata(L, lua_upvalueindex(1)); > + struct dict *bootenv = *(struct dict**)lua_touserdata(L, lua_upvalueindex(1)); I read the line twice but I do not yet understand the change, the two constructs are equivalent and your change makes it more difficult to understand. > const char *name = luaL_checkstring(L, 1); > const char *value = luaL_checkstring(L, 2); > > @@ -1036,7 +1036,8 @@ static int l_handler_wrapper(struct img_type *img, void *data) { > ERROR("Lua stack corrupted."); > return -1; > } > - lua_pushlightuserdata(gL, (void *)img->bootloader); > + struct dict **udbootenv = lua_newuserdata(gL, sizeof(struct dict*)); > + *udbootenv = img->bootloader; > luaL_setfuncs(gL, l_swupdate_bootenv, 1); > lua_pop(gL, 1); > } > @@ -1206,7 +1207,8 @@ lua_State *lua_parser_init(const char *buf, struct dict *bootenv) > lua_setglobal(L, "SWUPDATE_LUA_TYPE"); /* prime L as LUA_TYPE_PEMBSCR */ > luaL_openlibs(L); /* opens the standard libraries */ > luaL_requiref(L, "swupdate", luaopen_swupdate, 1 ); > - lua_pushlightuserdata(L, (void *)bootenv); > + struct dict **udbootenv = lua_newuserdata(L, sizeof(struct dict*)); > + *udbootenv = bootenv; These are not the only occurrencies of lua_pushlightuserdata(). Some lines above: lua_pushlightuserdata(gL, (void*)LUA_TYPE_HANDLER); lua_pushlightuserdata(L, (void*)LUA_TYPE_PEMBSCR); Don't they generate the same crash ? > luaL_setfuncs(L, l_swupdate_bootenv, 1); > lua_pop(L, 1); /* remove unused copy left on stack */ > > Best regards, Stefano Babic
Hi Stefano, > > Current aarch64 (distro) kernels ship with CONFIG_ARM64_VA_BITS_48 > > enabled, see, e.g., [1] for Debian's kernel, resulting in a LuaJIT > > panic "bad light userdata pointer" on aarch64 with distro-packaged > > old(er) LuaJIT versions that limit lightuserdata to 47 bit pointers. > > > > One mitigation option is to use a kernel with less VA bits, ruling > > out using distro kernels untouched. > > Another mitigation is to use the latest LuaJIT as this issue has > > been resolved, see [2,3], ruling out using the (older) distro's > > LuaJIT package untouched -- until distros eventually catch up. > > > > Hence, to allow SWUpdate on systems with >47 bits VA to run on current > > distros' kernel and LuaJIT packages, replace lightuserdata with "full" > > userdata. For other systems with with <=47 bits VA, this change has no > > effect other than Lua's gc having to clean up the "full" userdata of > > sizeof(struct dict*) when it becomes unreferenced: > > For sw-description embedded scripts, this is happens after having > > interpreted sw-description. > > For a Lua handler, this happens at its next call as an explicit free > > and gc cycle call would occupy more permanent space. > > > > Frankly speaking, this wants to fix an issue in another package into SWUpdate. Yes, true. > The right fix here is not to change kernel, but as you propose to update > LuaJIT that contains the fix. Sadly, distros don't catch up that fast and you have to package LuaJIT from a non-released version as of now. As this change adds no (big) burden to SWUpdate, I'd opt for doing this in SWUpdate... > Anyway, this can be merged into SWUpdate if there are not any > drawbacks. We have no specific performance issue to prefer > lighuserdata (that is pointer) to generic userdata. As far as I see it, there's no drawback except the memory management burden put on Lua by using "full" userdata ― including garbage collection. Performance-wise, this doesn't add much except for the memory allocation in Lua and its garbage collection. The former one would do upfront and the latter one one would try to avoid anyway in performance-critical use cases... > > [1] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsalsa.debian.org%2Fkernel-team%2Flinux%2F-%2Fblob%2Fbullseye%2Fdebian%2Fconfig%2Farm64%2Fconfig%23L13&data=04%7C01%7Cf12e6810-e622-4957-a0c8-c67ff4e321b6%40ad011.siemens.com%7Caa0911ec5905473729c408d96a184061%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637657474656831538%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=wA%2BgllioE%2F7EmtlJa2PSGwOBsoygzFV0%2FGmzeHz%2BhtI%3D&reserved=0 > > [2] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FLuaJIT%2FLuaJIT%2Fissues%2F49&data=04%7C01%7Cf12e6810-e622-4957-a0c8-c67ff4e321b6%40ad011.siemens.com%7Caa0911ec5905473729c408d96a184061%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637657474656841491%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=IVkQXVg9DgFn5p3OWFH9BGD2boLRPdlLi0shePaDulM%3D&reserved=0 > > [3] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FLuaJIT%2FLuaJIT%2Fpull%2F230&data=04%7C01%7Cf12e6810-e622-4957-a0c8-c67ff4e321b6%40ad011.siemens.com%7Caa0911ec5905473729c408d96a184061%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637657474656841491%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=dTq1gNjQ4go2JZ%2Bep%2BBdF4RKLaxrrCsv8LRfg7qoy9U%3D&reserved=0 > > > > Signed-off-by: Christian Storm <christian.storm@siemens.com> > > Signed-off-by: Michael Adler <michael.adler@siemens.com> > > --- > > corelib/lua_interface.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c > > index a6da6f2..e1a9c3e 100644 > > --- a/corelib/lua_interface.c > > +++ b/corelib/lua_interface.c > > @@ -854,7 +854,7 @@ static int l_get_bootenv(lua_State *L) { > > } > > static int l_set_bootenv(lua_State *L) { > > - struct dict *bootenv = (struct dict *)lua_touserdata(L, lua_upvalueindex(1)); > > + struct dict *bootenv = *(struct dict**)lua_touserdata(L, lua_upvalueindex(1)); > > I read the line twice but I do not yet understand the change, the two > constructs are equivalent and your change makes it more difficult to > understand. Hm, - (struct dict *) + *(struct dict**) is a change, semantically? > > const char *name = luaL_checkstring(L, 1); > > const char *value = luaL_checkstring(L, 2); > > @@ -1036,7 +1036,8 @@ static int l_handler_wrapper(struct img_type *img, void *data) { > > ERROR("Lua stack corrupted."); > > return -1; > > } > > - lua_pushlightuserdata(gL, (void *)img->bootloader); > > + struct dict **udbootenv = lua_newuserdata(gL, sizeof(struct dict*)); > > + *udbootenv = img->bootloader; > > luaL_setfuncs(gL, l_swupdate_bootenv, 1); > > lua_pop(gL, 1); > > } > > @@ -1206,7 +1207,8 @@ lua_State *lua_parser_init(const char *buf, struct dict *bootenv) > > lua_setglobal(L, "SWUPDATE_LUA_TYPE"); /* prime L as LUA_TYPE_PEMBSCR */ > > luaL_openlibs(L); /* opens the standard libraries */ > > luaL_requiref(L, "swupdate", luaopen_swupdate, 1 ); > > - lua_pushlightuserdata(L, (void *)bootenv); > > + struct dict **udbootenv = lua_newuserdata(L, sizeof(struct dict*)); > > + *udbootenv = bootenv; > > These are not the only occurrencies of lua_pushlightuserdata(). > > Some lines above: > > lua_pushlightuserdata(gL, (void*)LUA_TYPE_HANDLER); > lua_pushlightuserdata(L, (void*)LUA_TYPE_PEMBSCR); > > Don't they generate the same crash ? Yes, there are. We can opt for also replacing them for consistency but it's not strictly needed: The values (LUA_TYPE_PEMBSCR=1 and LUA_TYPE_HANDLER=2) are too small to run into the VA size problem and they're *not* dereferenced but value-compared (via the function bool is_type()). So, I'd opt for keeping them around... Kind regards, Christian
Hi Christian, On 02.09.21 17:00, Christian Storm wrote: > Hi Stefano, > >>> Current aarch64 (distro) kernels ship with CONFIG_ARM64_VA_BITS_48 >>> enabled, see, e.g., [1] for Debian's kernel, resulting in a LuaJIT >>> panic "bad light userdata pointer" on aarch64 with distro-packaged >>> old(er) LuaJIT versions that limit lightuserdata to 47 bit pointers. >>> >>> One mitigation option is to use a kernel with less VA bits, ruling >>> out using distro kernels untouched. >>> Another mitigation is to use the latest LuaJIT as this issue has >>> been resolved, see [2,3], ruling out using the (older) distro's >>> LuaJIT package untouched -- until distros eventually catch up. >>> >>> Hence, to allow SWUpdate on systems with >47 bits VA to run on current >>> distros' kernel and LuaJIT packages, replace lightuserdata with "full" >>> userdata. For other systems with with <=47 bits VA, this change has no >>> effect other than Lua's gc having to clean up the "full" userdata of >>> sizeof(struct dict*) when it becomes unreferenced: >>> For sw-description embedded scripts, this is happens after having >>> interpreted sw-description. >>> For a Lua handler, this happens at its next call as an explicit free >>> and gc cycle call would occupy more permanent space. >>> >> >> Frankly speaking, this wants to fix an issue in another package into SWUpdate. > > Yes, true. > >> The right fix here is not to change kernel, but as you propose to update >> LuaJIT that contains the fix. > > Sadly, distros don't catch up that fast and you have to package LuaJIT > from a non-released version as of now. As this change adds no (big) burden > to SWUpdate, I'd opt for doing this in SWUpdate... > OK, if there are n o drawbacks, we can do int this way. >> Anyway, this can be merged into SWUpdate if there are not any >> drawbacks. We have no specific performance issue to prefer >> lighuserdata (that is pointer) to generic userdata. > > As far as I see it, there's no drawback except the memory management > burden put on Lua by using "full" userdata ― including garbage collection. > Ok, fine. > Performance-wise, this doesn't add much except for the memory allocation > in Lua and its garbage collection. The former one would do upfront and > the latter one one would try to avoid anyway in performance-critical use > cases... This is not a problem, too. > > >>> [1] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsalsa.debian.org%2Fkernel-team%2Flinux%2F-%2Fblob%2Fbullseye%2Fdebian%2Fconfig%2Farm64%2Fconfig%23L13&data=04%7C01%7Cf12e6810-e622-4957-a0c8-c67ff4e321b6%40ad011.siemens.com%7Caa0911ec5905473729c408d96a184061%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637657474656831538%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=wA%2BgllioE%2F7EmtlJa2PSGwOBsoygzFV0%2FGmzeHz%2BhtI%3D&reserved=0 >>> [2] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FLuaJIT%2FLuaJIT%2Fissues%2F49&data=04%7C01%7Cf12e6810-e622-4957-a0c8-c67ff4e321b6%40ad011.siemens.com%7Caa0911ec5905473729c408d96a184061%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637657474656841491%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=IVkQXVg9DgFn5p3OWFH9BGD2boLRPdlLi0shePaDulM%3D&reserved=0 >>> [3] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FLuaJIT%2FLuaJIT%2Fpull%2F230&data=04%7C01%7Cf12e6810-e622-4957-a0c8-c67ff4e321b6%40ad011.siemens.com%7Caa0911ec5905473729c408d96a184061%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637657474656841491%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=dTq1gNjQ4go2JZ%2Bep%2BBdF4RKLaxrrCsv8LRfg7qoy9U%3D&reserved=0 >>> >>> Signed-off-by: Christian Storm <christian.storm@siemens.com> >>> Signed-off-by: Michael Adler <michael.adler@siemens.com> >>> --- >>> corelib/lua_interface.c | 8 +++++--- >>> 1 file changed, 5 insertions(+), 3 deletions(-) >>> >>> diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c >>> index a6da6f2..e1a9c3e 100644 >>> --- a/corelib/lua_interface.c >>> +++ b/corelib/lua_interface.c >>> @@ -854,7 +854,7 @@ static int l_get_bootenv(lua_State *L) { >>> } >>> static int l_set_bootenv(lua_State *L) { >>> - struct dict *bootenv = (struct dict *)lua_touserdata(L, lua_upvalueindex(1)); >>> + struct dict *bootenv = *(struct dict**)lua_touserdata(L, lua_upvalueindex(1)); >> >> I read the line twice but I do not yet understand the change, the two >> constructs are equivalent and your change makes it more difficult to >> understand. > > Hm, > - (struct dict *) > + *(struct dict**) > is a change, semantically? It is, but lua_touserdata returns a pointer (a void * that must be casted), and our structure is a struct dict *. So I do not understand why it is casted first to a struct dict** and then we get what it points, as this results exactly as in the original one. > > >>> const char *name = luaL_checkstring(L, 1); >>> const char *value = luaL_checkstring(L, 2); >>> @@ -1036,7 +1036,8 @@ static int l_handler_wrapper(struct img_type *img, void *data) { >>> ERROR("Lua stack corrupted."); >>> return -1; >>> } >>> - lua_pushlightuserdata(gL, (void *)img->bootloader); >>> + struct dict **udbootenv = lua_newuserdata(gL, sizeof(struct dict*)); >>> + *udbootenv = img->bootloader; >>> luaL_setfuncs(gL, l_swupdate_bootenv, 1); >>> lua_pop(gL, 1); >>> } >>> @@ -1206,7 +1207,8 @@ lua_State *lua_parser_init(const char *buf, struct dict *bootenv) >>> lua_setglobal(L, "SWUPDATE_LUA_TYPE"); /* prime L as LUA_TYPE_PEMBSCR */ >>> luaL_openlibs(L); /* opens the standard libraries */ >>> luaL_requiref(L, "swupdate", luaopen_swupdate, 1 ); >>> - lua_pushlightuserdata(L, (void *)bootenv); >>> + struct dict **udbootenv = lua_newuserdata(L, sizeof(struct dict*)); >>> + *udbootenv = bootenv; >> >> These are not the only occurrencies of lua_pushlightuserdata(). >> >> Some lines above: >> >> lua_pushlightuserdata(gL, (void*)LUA_TYPE_HANDLER); >> lua_pushlightuserdata(L, (void*)LUA_TYPE_PEMBSCR); >> >> Don't they generate the same crash ? > > Yes, there are. We can opt for also replacing them for consistency but > it's not strictly needed: The values (LUA_TYPE_PEMBSCR=1 and > LUA_TYPE_HANDLER=2) are too small to run into the VA size problem and > they're *not* dereferenced but value-compared (via the function bool > is_type()). So, I'd opt for keeping them around... Ok, fine with me. Best regards, Stefano > > > Kind regards, > Christian >
Hi Stefano, > > > > [...] > > > > diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c > > > > index a6da6f2..e1a9c3e 100644 > > > > --- a/corelib/lua_interface.c > > > > +++ b/corelib/lua_interface.c > > > > @@ -854,7 +854,7 @@ static int l_get_bootenv(lua_State *L) { > > > > } > > > > static int l_set_bootenv(lua_State *L) { > > > > - struct dict *bootenv = (struct dict *)lua_touserdata(L, lua_upvalueindex(1)); > > > > + struct dict *bootenv = *(struct dict**)lua_touserdata(L, lua_upvalueindex(1)); > > > > > > I read the line twice but I do not yet understand the change, the two > > > constructs are equivalent and your change makes it more difficult to > > > understand. > > > > Hm, > > - (struct dict *) > > + *(struct dict**) > > is a change, semantically? > > It is, but lua_touserdata returns a pointer (a void * that must be > casted), and our structure is a struct dict *. So I do not understand > why it is casted first to a struct dict** and then we get what it > points, as this results exactly as in the original one. The original | struct dict *bootenv = (struct dict *)lua_touserdata(L, lua_upvalueindex(1)); means bootenv points to the memory allocated by the lua_newuserdata() calls below. Stored therein is the pointer to the actual struct dict to which bootenv should point to, so it needs to be dereferenced. With | *(struct dict**) it's casted to a double pointer and then dereferenced so that bootenv points to the actual struct dict and not the memory block allocated by lua_newuserdata(). It could've been also written something like | intptr_t *mem = (intptr_t*)lua_touserdata(L, lua_upvalueindex(1)); | intptr_t bootenv_p = *mem; | struct dict *bootenv = (struct dict*)bootenv_p; i.e., you need the dereference / double pointer as the actual data is not at lua_touserdata(L, lua_upvalueindex(1)) but this gives you the pointer to the actual data. > > > > const char *name = luaL_checkstring(L, 1); > > > > const char *value = luaL_checkstring(L, 2); > > > > @@ -1036,7 +1036,8 @@ static int l_handler_wrapper(struct img_type *img, void *data) { > > > > ERROR("Lua stack corrupted."); > > > > return -1; > > > > } > > > > - lua_pushlightuserdata(gL, (void *)img->bootloader); > > > > + struct dict **udbootenv = lua_newuserdata(gL, sizeof(struct dict*)); > > > > + *udbootenv = img->bootloader; > > > > luaL_setfuncs(gL, l_swupdate_bootenv, 1); > > > > lua_pop(gL, 1); > > > > } > > > > @@ -1206,7 +1207,8 @@ lua_State *lua_parser_init(const char *buf, struct dict *bootenv) > > > > lua_setglobal(L, "SWUPDATE_LUA_TYPE"); /* prime L as LUA_TYPE_PEMBSCR */ > > > > luaL_openlibs(L); /* opens the standard libraries */ > > > > luaL_requiref(L, "swupdate", luaopen_swupdate, 1 ); > > > > - lua_pushlightuserdata(L, (void *)bootenv); > > > > + struct dict **udbootenv = lua_newuserdata(L, sizeof(struct dict*)); > > > > + *udbootenv = bootenv; Best regards, Christian
diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c index a6da6f2..e1a9c3e 100644 --- a/corelib/lua_interface.c +++ b/corelib/lua_interface.c @@ -854,7 +854,7 @@ static int l_get_bootenv(lua_State *L) { } static int l_set_bootenv(lua_State *L) { - struct dict *bootenv = (struct dict *)lua_touserdata(L, lua_upvalueindex(1)); + struct dict *bootenv = *(struct dict**)lua_touserdata(L, lua_upvalueindex(1)); const char *name = luaL_checkstring(L, 1); const char *value = luaL_checkstring(L, 2); @@ -1036,7 +1036,8 @@ static int l_handler_wrapper(struct img_type *img, void *data) { ERROR("Lua stack corrupted."); return -1; } - lua_pushlightuserdata(gL, (void *)img->bootloader); + struct dict **udbootenv = lua_newuserdata(gL, sizeof(struct dict*)); + *udbootenv = img->bootloader; luaL_setfuncs(gL, l_swupdate_bootenv, 1); lua_pop(gL, 1); } @@ -1206,7 +1207,8 @@ lua_State *lua_parser_init(const char *buf, struct dict *bootenv) lua_setglobal(L, "SWUPDATE_LUA_TYPE"); /* prime L as LUA_TYPE_PEMBSCR */ luaL_openlibs(L); /* opens the standard libraries */ luaL_requiref(L, "swupdate", luaopen_swupdate, 1 ); - lua_pushlightuserdata(L, (void *)bootenv); + struct dict **udbootenv = lua_newuserdata(L, sizeof(struct dict*)); + *udbootenv = bootenv; luaL_setfuncs(L, l_swupdate_bootenv, 1); lua_pop(L, 1); /* remove unused copy left on stack */