[PATCH] Lua: Replace lightuserdata to fix LuaJIT on aarch64 with 48bit VA
227 views
Skip to first unread message
Christian Storm
Aug 27, 2021, 10:50:00 AM8/27/21
to swup...@googlegroups.com, Christian Storm, Michael Adler
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.
[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 <christi...@siemens.com>
Signed-off-by: Michael Adler <michae...@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));
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 */
--
2.33.0
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.
[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 <christi...@siemens.com>
Signed-off-by: Michael Adler <michae...@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));
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 */
--
2.33.0
Stefano Babic
Aug 28, 2021, 7:37:44 AM8/28/21
to Christian Storm, swup...@googlegroups.com, Michael Adler
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 <christi...@siemens.com>
> Signed-off-by: Michael Adler <michae...@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
--
=====================================================================
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sba...@denx.de
=====================================================================
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.
>
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 <christi...@siemens.com>
> Signed-off-by: Michael Adler <michae...@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));
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;
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 */
>
>
Stefano Babic
--
=====================================================================
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sba...@denx.de
=====================================================================
Christian Storm
Sep 2, 2021, 11:00:12 AM9/2/21
to swup...@googlegroups.com
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
- (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
--
Dr. Christian Storm
Siemens AG, Technology, T RDA IOT SES-DE
Otto-Hahn-Ring 6, 81739 München, Germany
> > 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.
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.
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 <christi...@siemens.com>
> > Signed-off-by: Michael Adler <michae...@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,
> > Signed-off-by: Christian Storm <christi...@siemens.com>
> > Signed-off-by: Michael Adler <michae...@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.
- (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 ?
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
--
Dr. Christian Storm
Siemens AG, Technology, T RDA IOT SES-DE
Otto-Hahn-Ring 6, 81739 München, Germany
Stefano Babic
Sep 6, 2021, 6:21:05 AM9/6/21
to swup...@googlegroups.com
Hi Christian,
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 <christi...@siemens.com>
>>> Signed-off-by: Michael Adler <michae...@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.
Ok, fine with me.
Best regards,
Stefano
>
>
> Kind regards,
> Christian
>> 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 <christi...@siemens.com>
>>> Signed-off-by: Michael Adler <michae...@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?
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.
Best regards,
Stefano
>
>
> Kind regards,
> Christian
Christian Storm
Sep 6, 2021, 10:23:21 AM9/6/21
to swup...@googlegroups.com
Hi Stefano,
> > > > [...]
| 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,
> > > > [...]
> > > > 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
> > > > 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.
| struct dict *bootenv = (struct dict *)lua_touserdata(L, lua_upvalueindex(1));
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;
Reply all
Reply to author
Forward
0 new messages