Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

llext: fix handling of unimplemented syscalls #79309

Merged
merged 1 commit into from
Oct 8, 2024
Merged

llext: fix handling of unimplemented syscalls #79309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions include/zephyr/linker/llext-sections.ld
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,17 @@
/* SPDX-License-Identifier: Apache-2.0 */

/*
* Map the no_syscall_impl symbol in llext_export_syscalls.c to
* absolute address 0 so other weak symbols are exported as NULL.
* This section is used for mapping that symbol only and is not
* to be included in the final binary.
*/

SECTION_PROLOGUE(llext_no_syscall_impl, 0 (COPY), )
{
*(llext_no_syscall_impl)
}

/*
* Special section used by LLEXT if CONFIG_LLEXT_EXPORT_BUILTINS_BY_SLID
* is enabled. Declare this section to prevent it from being considered orphan.
Expand Down
15 changes: 10 additions & 5 deletions scripts/build/gen_syscalls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,11 +159,16 @@


exported_template = """
/* Export syscalls for extensions */
static void * const no_handler = NULL;
/*
* This symbol is placed at address 0 by llext-sections.ld. Its value and
* type is not important, we are only interested in its location
*/
static void * const no_syscall_impl Z_GENERIC_SECTION(llext_no_syscall_impl);

/* Weak references, if something is not found by the linker, it will be NULL
* and simply fail during extension load
/*
* Weak references to all syscall implementations. Those not found by the
* linker outside this file will be exported as NULL and simply fail when
* an extension requiring them is loaded.
*/
%s

Expand Down Expand Up @@ -495,7 +500,7 @@ def main():
if args.syscall_export_llext:
with open(args.syscall_export_llext, "w") as fp:
# Export symbols for emitted syscalls
weak_refs = "\n".join("extern __weak ALIAS_OF(no_handler) void * const %s;"
weak_refs = "\n".join("extern __weak ALIAS_OF(no_syscall_impl) void * const %s;"
% e for e in exported)
exported_symbols = "\n".join("EXPORT_SYMBOL(%s);"
% e for e in exported)
Expand Down
11 changes: 5 additions & 6 deletions tests/subsys/llext/simple/src/test_llext_simple.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,17 +490,16 @@ ZTEST(llext, test_printk_exported)
}

/*
* Ensure ext_syscall_fail is exported - as it is picked up by the syscall
* build machinery - but points to NULL as it is not implemented.
* The syscalls test above verifies that custom syscalls defined by extensions
* are properly exported. Since `ext_syscalls.h` declares ext_syscall_fail, we
* know it is picked up by the syscall build machinery, but the implementation
* for it is missing. Make sure the exported symbol for it is NULL.
*/
ZTEST(llext, test_ext_syscall_fail)
{
const void * const esf_fn = LLEXT_FIND_BUILTIN_SYM(z_impl_ext_syscall_fail);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

asserting est_fn is not null should still be done here no?

Copy link
Collaborator Author

@pillo79 pillo79 Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, llext_find_sym returns pointers to actual functions/data, so it makes sense for esf_fn to be NULL.

As it is currently done in main, and only in the case of unimplemented syscalls, the pointer returned by llext_find_sym points to a pointer to NULL (which is what was being tested here). But I assume this was a mistake, since:

  1. the stored address is not NULL, so LLEXT happily links the code
  2. at runtime, actual calls to an unimplemented z_impl_foo() jump at the location of a void *z_impl_foo = NULL, and the CPU fetches 4 or 8 zeroes as instructions. 💣

With this PR, the stored address is NULL, find_sym returns that NULL (not "no symbol", but "that symbol is defined and its value is NULL"), and the link process fails (as it should, there is no implementation for that symbol after all).

zassert_not_null(esf_fn, "est_fn should not be NULL");

zassert_is_null(*(uintptr_t **)esf_fn, NULL,
"ext_syscall_fail should be NULL");
zassert_is_null(esf_fn, "est_fn should be NULL");
}

ZTEST_SUITE(llext, NULL, NULL, NULL, NULL, NULL);
Loading