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.

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

libc: Implement userland implementations of exit functions #6197

Merged
merged 3 commits into from
May 25, 2022

Conversation

pussuw
Copy link
Contributor

@pussuw pussuw commented May 3, 2022

Summary

Implement atexit(), on_exit(), exit() to libc. Using the call gate versions is problematic as they basically inject userland code into the kernel, which is then run with kernel privileges. In any case the functions belong to libc.

Impact

Exit functions are executed with user privileges

Testing

mpfs icicle:knsh

@pussuw pussuw marked this pull request as draft May 3, 2022 08:30
@pussuw

This comment was marked as outdated.

@pussuw pussuw force-pushed the libc_exitfuncs branch 5 times, most recently from bec0dea to 3de7365 Compare May 4, 2022 10:16
sched/task/exit.c Outdated Show resolved Hide resolved
@pussuw pussuw force-pushed the libc_exitfuncs branch 3 times, most recently from 6da361a to 937cfc9 Compare May 4, 2022 11:30
@pussuw

This comment was marked as resolved.

@Ouss4
Copy link
Member

Ouss4 commented May 4, 2022

@pussuw there seem to be some warnings:

stdlib/lib_cxa_atexit.c:83:13: error: '__cxa_callback' defined but not used [-Werror=unused-function]

CI turns warnings into errors, I don't know if you do that too, but you can with: make EXTRAFLAGS="-Werror".

(there are some make[3]: arm-nuttx-eabi-gcc: Command not found as well, there is an issue with the script apparently, please ignore it for now.)

@pussuw

This comment was marked as resolved.

@Ouss4
Copy link
Member

Ouss4 commented May 4, 2022

The other build error which I listed above is way beyond my understanding. The make recipe works locally for the targets that fail in CI.

It could be a parallel make issue as it seemed that the dependencies were messed up. I took a quick look at libs/Makefile, nothing stands out. I hope (sorry :) ) that it can be reproduced once the warning is fixed.

@Ouss4
Copy link
Member

Ouss4 commented May 4, 2022

@pussuw Apparently we have a name collision with https://github.com/apache/incubator-nuttx/blob/master/libs/libc/stdlib/lib_Exit.c and your newly created file lib_exit.c. I won't pretend that I understand the case sensitivity of all file systems, but I least am not able to create lib_exit.c under MacOS when lib_Exit.c exists.

@pussuw
Copy link
Contributor Author

pussuw commented May 4, 2022

@pussuw Apparently we have a name collision with https://github.com/apache/incubator-nuttx/blob/master/libs/libc/stdlib/lib_Exit.c and your newly created file lib_exit.c. I won't pretend that I understand the case sensitivity of all file systems, but I least am not able to create lib_exit.c under MacOS when lib_Exit.c exists.

@Ouss4 thanks for doing the investigation! That makes sense then, since I don't get the error but I'm on Linux in which file names are case sensitive. I'll merge the two files, makes no sense to have two.

@pussuw

This comment was marked as resolved.

@pussuw pussuw force-pushed the libc_exitfuncs branch 2 times, most recently from 5939b55 to 968d200 Compare May 5, 2022 06:52
@pussuw

This comment was marked as duplicate.

@pussuw pussuw force-pushed the libc_exitfuncs branch from 968d200 to a9169dc Compare May 5, 2022 07:20
@pussuw pussuw marked this pull request as ready for review May 5, 2022 07:29
libs/libc/stdlib/lib_atexit.c Outdated Show resolved Hide resolved
@pussuw pussuw force-pushed the libc_exitfuncs branch 3 times, most recently from dfb1b2e to 4f79869 Compare May 5, 2022 09:56
@pussuw

This comment was marked as resolved.

@pussuw pussuw force-pushed the libc_exitfuncs branch from 4f79869 to cb19ef1 Compare May 5, 2022 12:04
@pkarashchenko
Copy link
Contributor

Seems like something on the ARM side still pulls __dso_handle and since cxa_atexit.c is not compiled the weak reference is not there. Need to move the weak symbol declaration somewhere else.

arm-none-eabi-ld: /github/workspace/sources/nuttx/staging/libapps.a(symtab.c.github.workspace.sources.apps.examples.elf.o):(.rodata.g_elf_exports+0x1c): undefined reference to `__dso_handle'

I still think that we are missing extern

pussuw added 3 commits May 25, 2022 08:59
For CONFIG_BUILD_KERNEL using the sched/task/task_exithook implementation
will just not work. It calls user code with kernel privileges which is
a bit of a security issue.
Remove the kernel side implementations altogether. These will be
replaced by user land implementations.
Copy link
Contributor

@pkarashchenko pkarashchenko left a comment

Choose a reason for hiding this comment

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

LGTM and great work!

@xiaoxiang781216 xiaoxiang781216 merged commit 5800dd2 into apache:master May 25, 2022
@pussuw pussuw deleted the libc_exitfuncs branch May 25, 2022 07:39
@pussuw
Copy link
Contributor Author

pussuw commented May 25, 2022

Thanks for the review, turned into quite a magnus opus in the end.

@masayuki2009
Copy link
Contributor

@Ouss4

I noticed that the ostest with esp32-devkitc:smp (real board not qemu) causes a deadlock.

user_main: waitpid test

Test waitpid()
waitpid_main: PID 28 Started
waitpid_start_child: Started waitpid_main at PID=28
waitpid_main: PID 29 Started
waitpid_start_child: Started waitpid_main at PID=29
waitpid_main: PID 30 Started
waitpid_start_child: Started waitpid_main at PID=30
waitpid_test: Waiting for PID=28 with waitpid()
waitpid_main: PID 28 exitting with result=14
waitpid_main: PID 29 exitting with result=14
waitpid_test: PID 28 waitpid succeeded with stat_loc=0e00
waitpid_last: Waiting for PID=30 with waitpid()
waitpid_main: PID 30 exitting with result=14
waitpid_last: PASS: PID 30 waitpid succeeded with stat_loc=0e00

End of test memory usage:
VARIABLE  BEFORE   AFTER
======== ======== ========
arena       4cda0    4cda0
ordblks         5       12
mxordblk    2b730    2b730
uordblks     b960     bce0
fordblks    41440    410c0

user_main: mutex test
Initializing mutex
Starting thread 1
Starting thread 2

@Ouss4
Copy link
Member

Ouss4 commented May 25, 2022

@masayuki2009 did you enable any other option?
I just tried quickly running ostest 2 times and both passed.

Actually, I've been using the latest master the whole day for some tests.

@masayuki2009
Copy link
Contributor

@masayuki2009 did you enable any other option?

No

I just tried quickly running ostest 2 times and both passed.
Actually, I've been using the latest master the whole day for some tests.

Hmm, I confirmed that the latest upstream works.
It seems that #6286 fixed the issue.
However, I'm still not understand why the PR fixed the issue.

@xiaoxiang781216
Copy link
Contributor

Yes, it's strange. #6286 could improve the performance which may reduce the race condition.

@xiaoxiang781216 xiaoxiang781216 added the breaking change This change requires a mitigation entry in the release notes. label Jun 5, 2022
yamt added a commit to yamt/incubator-nuttx-apps that referenced this pull request Dec 18, 2024
xiaoxiang781216 pushed a commit to apache/nuttx-apps that referenced this pull request Dec 18, 2024
jerpelea pushed a commit to jerpelea/nuttx-apps that referenced this pull request Dec 19, 2024
jerpelea pushed a commit to jerpelea/nuttx-apps that referenced this pull request Dec 19, 2024
xiaoxiang781216 pushed a commit to apache/nuttx-apps that referenced this pull request Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This change requires a mitigation entry in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants