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

arch: riscv: stacktrace: fix user thread stack bound check #75564

Merged

Conversation

ycsin
Copy link
Member

@ycsin ycsin commented Jul 8, 2024

According to the riscv's arch.h:

 +------------+ <- thread.arch.priv_stack_start
 | Guard      | } Z_RISCV_STACK_GUARD_SIZE
 +------------+
 | Priv Stack | } CONFIG_PRIVILEGED_STACK_SIZE
 +------------+ <- thread.arch.priv_stack_start +
                   CONFIG_PRIVILEGED_STACK_SIZE +
                   Z_RISCV_STACK_GUARD_SIZE

The start of the privilege stack should be:

thread.arch.priv_stack_start + Z_RISCV_STACK_GUARD_SIZE

Instead of

thread.arch.priv_stack_start - CONFIG_PRIVILEGED_STACK_SIZE

For the end, use the same equation of top_of_priv_stack in the arch_user_mode_enter()

@ycsin ycsin added the bug The issue is a bug, or the PR is fixing a bug label Jul 8, 2024
@ycsin ycsin added this to the v3.7.0 milestone Jul 8, 2024
@zephyrbot zephyrbot added the area: RISCV RISCV Architecture (32-bit & 64-bit) label Jul 8, 2024
@npitre
Copy link
Collaborator

npitre commented Jul 10, 2024

It might be better not to rely too much on CONFIG_PRIVILEGED_STACK_SIZE
directly as it ends up being subjected to extra rounding and alignment
through Z_STACK_PTR_ALIGN(). Might be worth to double-check against the
actual code in setup_thread_stack() and arch_user_mode_enter().

@ycsin
Copy link
Member Author

ycsin commented Jul 11, 2024

Thanks for the insights

It might be better not to rely too much on CONFIG_PRIVILEGED_STACK_SIZE
directly as it ends up being subjected to extra rounding and alignment
through Z_STACK_PTR_ALIGN().

Should it be

Z_STACK_PTR_ALIGN(_current->arch.priv_stack_start + K_KERNEL_STACK_RESERVED + CONFIG_PRIVILEGED_STACK_SIZE);

instead? According to

top_of_priv_stack = Z_STACK_PTR_ALIGN(_current->arch.priv_stack_start +
K_KERNEL_STACK_RESERVED +
CONFIG_PRIVILEGED_STACK_SIZE);

@npitre
Copy link
Collaborator

npitre commented Jul 11, 2024

If it is the privileged/exception stack for user threads that you need then
arch_curr_cpu()->arch.user_exc_sp would be its top.

@npitre
Copy link
Collaborator

npitre commented Jul 11, 2024

So yes, that's pretty much equivalent.

@npitre
Copy link
Collaborator

npitre commented Jul 11, 2024

Maybe this ought to be abstracted in a single location eventually.
It is too easy to change one spot and miss the other otherwise.

@ycsin
Copy link
Member Author

ycsin commented Jul 11, 2024

Maybe this ought to be abstracted in a single location eventually. It is too easy to change one spot and miss the other otherwise.

what if we store it somewhere, just like what we did with the stack_info?

According to the riscv's `arch.h`:

 +------------+ <- thread.arch.priv_stack_start
 | Guard      | } Z_RISCV_STACK_GUARD_SIZE
 +------------+
 | Priv Stack | } CONFIG_PRIVILEGED_STACK_SIZE
 +------------+ <- thread.arch.priv_stack_start +
                   CONFIG_PRIVILEGED_STACK_SIZE +
                   Z_RISCV_STACK_GUARD_SIZE

The start of the privilege stack should be:

  `thread.arch.priv_stack_start + Z_RISCV_STACK_GUARD_SIZE`

Instead of

  `thread.arch.priv_stack_start - CONFIG_PRIVILEGED_STACK_SIZE`

For the `end`, use the same equation of `top_of_priv_stack` in
the `arch_user_mode_enter()`

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
@ycsin ycsin force-pushed the pr/fix_riscv_stacktrace_priv_stack_bound branch from 292a6c7 to 4557fe0 Compare July 12, 2024 14:06
@ycsin ycsin requested a review from cfriedt July 15, 2024 05:51
@ycsin
Copy link
Member Author

ycsin commented Jul 16, 2024

ping @fkokosinski @kgugala @tgorochowik

Copy link
Member

@fkokosinski fkokosinski left a comment

Choose a reason for hiding this comment

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

LGTM

@nashif nashif added the backport v3.7-branch Request backport to the v3.7-branch label Jul 16, 2024
@nashif nashif modified the milestones: v3.7.0, v4.0.0 Jul 16, 2024
@nashif nashif merged commit 7db18ab into zephyrproject-rtos:main Jul 27, 2024
21 checks passed
@ycsin ycsin deleted the pr/fix_riscv_stacktrace_priv_stack_bound branch July 29, 2024 03:09
@ycsin ycsin linked an issue Jul 29, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: RISCV RISCV Architecture (32-bit & 64-bit) backport v3.7-branch Request backport to the v3.7-branch bug The issue is a bug, or the PR is fixing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

arch: riscv: stacktrace: user thread stack bound check is wrong
8 participants