You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "pussuw (via GitHub)" <gi...@apache.org> on 2023/01/24 12:24:44 UTC
[GitHub] [nuttx] pussuw opened a new pull request, #8227: arch/ARCH_KERNEL_STACK: Fix signal handling with kernel stack
pussuw opened a new pull request, #8227:
URL: https://github.com/apache/nuttx/pull/8227
## Summary
There were two issues with signal handling:
- With a kernel stack the "info" parameter was passed from kernel memory. This is fixed by making a stack frame to the user stack and copying it there.
- If the signal handler uses a system call, the kernel stack was completely and unconditionally destroyed, resulting in a crash in the user application
There is also no need to check ustkptr, it is always NULL. Why ? Because signal delivery is deferred when a system call is being executed.
## Impact
Fixes signal dispatch logic when CONFIG_ARCH_KERNEL_STACK=y
## Testing
icicle:knsh
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #8227: arch/ARCH_KERNEL_STACK: Fix signal handling with kernel stack
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8227:
URL: https://github.com/apache/nuttx/pull/8227#discussion_r1086485936
##########
arch/risc-v/src/common/riscv_swint.c:
##########
@@ -497,8 +515,16 @@ int riscv_swint(int irq, void *context, void *arg)
if (index == 0 && rtcb->xcp.kstack != NULL)
{
rtcb->xcp.ustkptr = (uintptr_t *)regs[REG_SP];
- regs[REG_SP] = (uintptr_t)rtcb->xcp.kstack +
+ if (rtcb->xcp.kstkptr == NULL)
Review Comment:
The change looks good could you apply the similar change to arm/xtensa?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pussuw commented on a diff in pull request #8227: arch/ARCH_KERNEL_STACK: Fix signal handling with kernel stack
Posted by "pussuw (via GitHub)" <gi...@apache.org>.
pussuw commented on code in PR #8227:
URL: https://github.com/apache/nuttx/pull/8227#discussion_r1085666671
##########
arch/risc-v/src/common/riscv_swint.c:
##########
@@ -497,8 +515,16 @@ int riscv_swint(int irq, void *context, void *arg)
if (index == 0 && rtcb->xcp.kstack != NULL)
{
rtcb->xcp.ustkptr = (uintptr_t *)regs[REG_SP];
- regs[REG_SP] = (uintptr_t)rtcb->xcp.kstack +
+ if (rtcb->xcp.kstkptr == NULL)
Review Comment:
The original check (== NULL) is actually correct. However, the original implementation is very difficult to follow and understand. I can't know how it was supposed to work, only that it was broken.
Without this check, i.e. unconditionally setting
`regs[REG_SP] = (uintptr_t)rtcb->xcp.kstack + ARCH_KERNEL_STACKSIZE;`
Destroys the entire kernel stack frame, because it resets the stack pointer to stack top. If rtcb->xcp.kstkptr == NULL we know that no one is using the kernel stack atm and we can set the stack to stack top. Otherwise we should not touch regs[REG_SP].
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pussuw commented on a diff in pull request #8227: arch/ARCH_KERNEL_STACK: Fix signal handling with kernel stack
Posted by "pussuw (via GitHub)" <gi...@apache.org>.
pussuw commented on code in PR #8227:
URL: https://github.com/apache/nuttx/pull/8227#discussion_r1086672666
##########
arch/arm/src/armv7-r/arm_syscall.c:
##########
@@ -434,11 +434,31 @@ uint32_t *arm_syscall(uint32_t *regs)
if (rtcb->xcp.kstack != NULL)
{
- DEBUGASSERT(rtcb->xcp.kstkptr == NULL &&
- rtcb->xcp.ustkptr != NULL);
+ uint32_t *usp;
Review Comment:
Yes you are correct, this will create a frame that is 4/8 times too big
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #8227: arch/ARCH_KERNEL_STACK: Fix signal handling with kernel stack
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8227:
URL: https://github.com/apache/nuttx/pull/8227#discussion_r1086664938
##########
arch/risc-v/src/common/riscv_swint.c:
##########
@@ -390,11 +389,31 @@ int riscv_swint(int irq, void *context, void *arg)
if (rtcb->xcp.kstack != NULL)
{
- DEBUGASSERT(rtcb->xcp.kstkptr == NULL &&
- rtcb->xcp.ustkptr != NULL);
+ uintptr_t *usp;
Review Comment:
need change to uint8_t *?
##########
arch/arm/src/armv7-r/arm_syscall.c:
##########
@@ -434,11 +434,31 @@ uint32_t *arm_syscall(uint32_t *regs)
if (rtcb->xcp.kstack != NULL)
{
- DEBUGASSERT(rtcb->xcp.kstkptr == NULL &&
- rtcb->xcp.ustkptr != NULL);
+ uint32_t *usp;
Review Comment:
need change to uint8_t *?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pussuw commented on a diff in pull request #8227: arch/ARCH_KERNEL_STACK: Fix signal handling with kernel stack
Posted by "pussuw (via GitHub)" <gi...@apache.org>.
pussuw commented on code in PR #8227:
URL: https://github.com/apache/nuttx/pull/8227#discussion_r1086496663
##########
arch/risc-v/src/common/riscv_swint.c:
##########
@@ -497,8 +515,16 @@ int riscv_swint(int irq, void *context, void *arg)
if (index == 0 && rtcb->xcp.kstack != NULL)
{
rtcb->xcp.ustkptr = (uintptr_t *)regs[REG_SP];
- regs[REG_SP] = (uintptr_t)rtcb->xcp.kstack +
+ if (rtcb->xcp.kstkptr == NULL)
Review Comment:
Yes of course!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #8227: arch/ARCH_KERNEL_STACK: Fix signal handling with kernel stack
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8227:
URL: https://github.com/apache/nuttx/pull/8227#discussion_r1086605889
##########
arch/risc-v/src/common/riscv_swint.c:
##########
@@ -497,8 +515,16 @@ int riscv_swint(int irq, void *context, void *arg)
if (index == 0 && rtcb->xcp.kstack != NULL)
{
rtcb->xcp.ustkptr = (uintptr_t *)regs[REG_SP];
- regs[REG_SP] = (uintptr_t)rtcb->xcp.kstack +
+ if (rtcb->xcp.kstkptr == NULL)
Review Comment:
Thanks @pussuw. I will merge your patch once it pass ci.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #8227: arch/ARCH_KERNEL_STACK: Fix signal handling with kernel stack
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8227:
URL: https://github.com/apache/nuttx/pull/8227#discussion_r1085651379
##########
arch/risc-v/src/common/riscv_swint.c:
##########
@@ -497,8 +515,16 @@ int riscv_swint(int irq, void *context, void *arg)
if (index == 0 && rtcb->xcp.kstack != NULL)
{
rtcb->xcp.ustkptr = (uintptr_t *)regs[REG_SP];
- regs[REG_SP] = (uintptr_t)rtcb->xcp.kstack +
+ if (rtcb->xcp.kstkptr == NULL)
Review Comment:
or change to DEBUGASSERT like line 453
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 merged pull request #8227: arch/ARCH_KERNEL_STACK: Fix signal handling with kernel stack
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 merged PR #8227:
URL: https://github.com/apache/nuttx/pull/8227
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pussuw commented on a diff in pull request #8227: arch/ARCH_KERNEL_STACK: Fix signal handling with kernel stack
Posted by "pussuw (via GitHub)" <gi...@apache.org>.
pussuw commented on code in PR #8227:
URL: https://github.com/apache/nuttx/pull/8227#discussion_r1086835522
##########
arch/arm64/src/common/arm64_syscall.c:
##########
@@ -436,11 +436,31 @@ int arm64_syscall(uint64_t *regs)
if (rtcb->xcp.kstack != NULL)
{
- DEBUGASSERT(rtcb->xcp.kstkptr == NULL &&
- rtcb->xcp.ustkptr != NULL);
+ uint32_t usp;
Review Comment:
Ofc, copy&paste error. I will fix tomorrow
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pussuw commented on a diff in pull request #8227: arch/ARCH_KERNEL_STACK: Fix signal handling with kernel stack
Posted by "pussuw (via GitHub)" <gi...@apache.org>.
pussuw commented on code in PR #8227:
URL: https://github.com/apache/nuttx/pull/8227#discussion_r1086443989
##########
arch/risc-v/src/common/riscv_swint.c:
##########
@@ -497,8 +515,16 @@ int riscv_swint(int irq, void *context, void *arg)
if (index == 0 && rtcb->xcp.kstack != NULL)
{
rtcb->xcp.ustkptr = (uintptr_t *)regs[REG_SP];
- regs[REG_SP] = (uintptr_t)rtcb->xcp.kstack +
+ if (rtcb->xcp.kstkptr == NULL)
Review Comment:
I spent a bit of time on this and I think the new patch should now do what this was originally intended to do:
- If rtcb->xcp.kstkptr is set, it means someone is already using the kernel stack and regs[SP] should be set to this
- If rtcb->xcp.kstkptr is not set, it means no one is using it, and regs[SP] should be set to the top of the kernel stack
Tested and verified the signal dispatch, and calling any system call from the signal handler still works
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pussuw commented on pull request #8227: arch/ARCH_KERNEL_STACK: Fix signal handling with kernel stack
Posted by "pussuw (via GitHub)" <gi...@apache.org>.
pussuw commented on PR #8227:
URL: https://github.com/apache/nuttx/pull/8227#issuecomment-1401853676
I will copy&paste the fix to the ARM targets, when the code passes review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #8227: arch/ARCH_KERNEL_STACK: Fix signal handling with kernel stack
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8227:
URL: https://github.com/apache/nuttx/pull/8227#discussion_r1085650256
##########
arch/risc-v/src/common/riscv_swint.c:
##########
@@ -497,8 +515,16 @@ int riscv_swint(int irq, void *context, void *arg)
if (index == 0 && rtcb->xcp.kstack != NULL)
{
rtcb->xcp.ustkptr = (uintptr_t *)regs[REG_SP];
- regs[REG_SP] = (uintptr_t)rtcb->xcp.kstack +
+ if (rtcb->xcp.kstkptr == NULL)
Review Comment:
or remove the check since it has been done at line 515
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pussuw commented on a diff in pull request #8227: arch/ARCH_KERNEL_STACK: Fix signal handling with kernel stack
Posted by "pussuw (via GitHub)" <gi...@apache.org>.
pussuw commented on code in PR #8227:
URL: https://github.com/apache/nuttx/pull/8227#discussion_r1086583214
##########
arch/risc-v/src/common/riscv_swint.c:
##########
@@ -497,8 +515,16 @@ int riscv_swint(int irq, void *context, void *arg)
if (index == 0 && rtcb->xcp.kstack != NULL)
{
rtcb->xcp.ustkptr = (uintptr_t *)regs[REG_SP];
- regs[REG_SP] = (uintptr_t)rtcb->xcp.kstack +
+ if (rtcb->xcp.kstkptr == NULL)
Review Comment:
Done for ARM.
I could not find kernel stack related code from xtensa or any other platform
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pussuw commented on a diff in pull request #8227: arch/ARCH_KERNEL_STACK: Fix signal handling with kernel stack
Posted by "pussuw (via GitHub)" <gi...@apache.org>.
pussuw commented on code in PR #8227:
URL: https://github.com/apache/nuttx/pull/8227#discussion_r1086672666
##########
arch/arm/src/armv7-r/arm_syscall.c:
##########
@@ -434,11 +434,31 @@ uint32_t *arm_syscall(uint32_t *regs)
if (rtcb->xcp.kstack != NULL)
{
- DEBUGASSERT(rtcb->xcp.kstkptr == NULL &&
- rtcb->xcp.ustkptr != NULL);
+ uint32_t *usp;
Review Comment:
Yes you are correct, this will create a frame that is 4/8 times too big, I will change this to unsigned long to do the raw arithmetics.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #8227: arch/ARCH_KERNEL_STACK: Fix signal handling with kernel stack
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8227:
URL: https://github.com/apache/nuttx/pull/8227#discussion_r1086814458
##########
arch/arm64/src/common/arm64_syscall.c:
##########
@@ -436,11 +436,31 @@ int arm64_syscall(uint64_t *regs)
if (rtcb->xcp.kstack != NULL)
{
- DEBUGASSERT(rtcb->xcp.kstkptr == NULL &&
- rtcb->xcp.ustkptr != NULL);
+ uint32_t usp;
Review Comment:
change to uint64_t? uint32_t isn't enough for arm64.
##########
arch/arm64/src/common/arm64_syscall.c:
##########
@@ -538,8 +557,15 @@ int arm64_syscall(uint64_t *regs)
if (index == 0 && rtcb->xcp.kstack != NULL)
{
rtcb->xcp.ustkptr = (uint32_t *)regs[REG_SP];
- regs[REG_SP] = (uint32_t)rtcb->xcp.kstack +
+ if (rtcb->xcp.kstkptr != NULL)
+ {
+ regs[REG_SP] = (uint32_t)rtcb->xcp.kstkptr;
Review Comment:
cast to uint64_t
##########
arch/arm64/src/common/arm64_syscall.c:
##########
@@ -538,8 +557,15 @@ int arm64_syscall(uint64_t *regs)
if (index == 0 && rtcb->xcp.kstack != NULL)
{
rtcb->xcp.ustkptr = (uint32_t *)regs[REG_SP];
- regs[REG_SP] = (uint32_t)rtcb->xcp.kstack +
+ if (rtcb->xcp.kstkptr != NULL)
+ {
+ regs[REG_SP] = (uint32_t)rtcb->xcp.kstkptr;
+ }
+ else
+ {
+ regs[REG_SP] = (uint32_t)rtcb->xcp.kstack +
Review Comment:
```suggestion
regs[REG_SP] = (uint64_t)rtcb->xcp.kstack +
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #8227: arch/ARCH_KERNEL_STACK: Fix signal handling with kernel stack
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8227:
URL: https://github.com/apache/nuttx/pull/8227#discussion_r1085649283
##########
arch/risc-v/src/common/riscv_swint.c:
##########
@@ -497,8 +515,16 @@ int riscv_swint(int irq, void *context, void *arg)
if (index == 0 && rtcb->xcp.kstack != NULL)
{
rtcb->xcp.ustkptr = (uintptr_t *)regs[REG_SP];
- regs[REG_SP] = (uintptr_t)rtcb->xcp.kstack +
+ if (rtcb->xcp.kstkptr == NULL)
Review Comment:
```suggestion
if (rtcb->xcp.kstkptr != NULL)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org