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

arm64: fix tpidr maybe null #15338

Merged
merged 1 commit into from
Dec 26, 2024
Merged

Conversation

hujun260
Copy link
Contributor

Note: Please adhere to Contributing Guidelines.

Summary

arm64: fix tpidr maybe null

Before the MPU initialization, the up_update_task(this_cpu()) function is called at a time when hardware cache coherency is not yet enabled. In certain critical scenarios, Core 1 reads a zero value for tcb from the global variable g_assignedtask and stores this zero value into the tpidr register. This results in subsequent interrupt handlers reading a zero tcb, causing an exception.

Impact

arm64

Testing

qemu-armv8a:nsh_smp

@github-actions github-actions bot added Arch: arm64 Issues related to ARM64 (64-bit) architecture Size: S The size of the change in this PR is small labels Dec 25, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 25, 2024

[Experimental Bot, please feedback here]

Yes, this PR likely meets the basic NuttX requirements but is missing crucial information.

Here's why it's borderline and what's missing:

  • Summary: While it explains the why and what, it lacks a clear explanation of how the fix works. What code was changed to prevent the zero value from being stored? This is the most important part of the summary.
  • Impact: Stating "arm64" is insufficient. It needs to specify:
    • Is new feature added? Is existing feature changed? (This is a bug fix, so existing feature changed)
    • Impact on user: Likely NO, but explicitly state it.
    • Impact on build: Likely NO, but explicitly state it.
    • Impact on hardware: YES, arm64. Be specific which arm64 chips/boards are affected.
    • Impact on documentation: Likely NO, but explicitly state it unless documentation was updated to reflect the issue/fix.
    • Impact on security: YES, potentially. Explain the security implications of a null tpidr. A crash due to a null pointer exception is a security issue if it can be triggered by user input or external events.
    • Impact on compatibility: Likely NO, but explicitly state it.
  • Testing: "qemu-armv8a:nsh_smp" is insufficient. Provide:
    • Build Host: Your OS, CPU architecture, compiler, and version.
    • MORE detailed Target: Specify the QEMU version and the exact configuration used.
    • Crucially missing: Logs. There are no "before" and "after" logs demonstrating the fix. This is a critical requirement. Show the error occurring before the fix and the successful operation after the fix. Ideally, the logs should demonstrate the tpidr value being correctly populated.

To make this PR acceptable, you need to add the missing details outlined above. Specifically, clarify how the fix works, expand on the impact section answering all the questions, and provide detailed build host information and actual before/after logs demonstrating the fix.

Before the MPU initialization, the up_update_task(this_cpu()) function is called at a time when hardware cache coherency is not yet enabled.
In certain critical scenarios, Core 1 reads a zero value for tcb from the global variable g_assignedtask and stores this zero value into the tpidr
register. This results in subsequent interrupt handlers reading a zero tcb, causing an exception.

Signed-off-by: hujun5 <[email protected]>

/* Init idle task to percpu reg */

up_update_task(current_task(this_cpu()));
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need change other arch too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the current community code implements percpu storage for this_task on arm64, armv7a, armv7r, and armv8r archs, all using this approach.

@xiaoxiang781216 xiaoxiang781216 merged commit 2886fdd into apache:master Dec 26, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm64 Issues related to ARM64 (64-bit) architecture Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants