Skip to content

Conversation

@XuNeo
Copy link
Contributor

@XuNeo XuNeo commented Sep 29, 2024

Summary

  1. Fix the unlock order f859c03
  2. Replace critical section with spinlock
  3. Bypass sched_lock from interrupt context

Impact

Now can call nxsched_smp_call from interrupt context. It's will be used to call function on all foreign CPUs from assert handler in order to stop all CPUs.

Testing

arm64 qemu with ostest apache/nuttx-apps#2628

  1. Build
    cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=1 -Bbuild -GNinja -DBOARD_CONFIG=boards/arm64/qemu/qemu-armv8a/configs/nsh_smp nuttx
  2. Launch
    qemu-system-aarch64 -smp 4 -cpu cortex-a53 -semihosting -nographic -machine virt,virtualization=on,gic-version=3 -net none -chardev stdio,id=con,mux=on -serial chardev:con -mon chardev=con,mode=readline -kernel build/nuttx -s
  3. Run ostest
    ostest &

dulibo1 and others added 2 commits September 29, 2024 13:43
1.nxsem_post wake up  nxsched_smp_call;
2.stack smp_call_data_s may return;
3.nxsched_smp_call_handler access call_data->lock is not safety;
so adjust the unlock order

Signed-off-by: dulibo1 <dulibo1@xiaomi.com>
Signed-off-by: xuxingliang <xuxingliang@xiaomi.com>
@github-actions github-actions bot added Area: OS Components OS Components issues Size: S The size of the change in this PR is small labels Sep 29, 2024
@nuttxpr
Copy link

nuttxpr commented Sep 29, 2024

[Experimental Bot, please feedback here]

The PR appears to meet the NuttX requirements, but with a caveat:

Strengths:

  • Clear Summary: Explains the "why," "what," and "how" of the changes. The related commit hash is helpful for context but ideally would be a linked PR for easier tracking.
  • Impact: Addresses most relevant points. The impact on users, build process, and documentation seems minimal. However, the impact on security is flagged ("Now can call nxsched_smp_call from interrupt context") and warrants further scrutiny.
  • Testing: Provides a specific test setup, steps to reproduce, and mentions the related NuttX apps PR. This makes it easier for reviewers to verify the changes.

Areas for Improvement:

  • Impact (Security): Calling functions across CPUs from an interrupt context can have significant security implications. The PR should explicitly address:
    • What security risks were present with the previous unlock order?
    • How does using a spinlock and bypassing sched_lock mitigate those risks?
    • Have any new potential vulnerabilities been introduced?
    • Ideally, code review should involve security experts.
  • Testing Logs: While the steps to reproduce are helpful, including actual testing logs (even if brief) demonstrating the issue before and the fix after would strengthen the PR significantly.

Overall: The PR provides a good foundation but needs to elaborate on the security implications and ideally include testing logs for a more robust review.

@xiaoxiang781216 xiaoxiang781216 merged commit a9b8681 into apache:master Sep 30, 2024
@XuNeo XuNeo deleted the smp-call-from-interrupt branch September 30, 2024 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: OS Components OS Components issues 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.

4 participants