nuttxpr commented on PR #15461: URL: https://github.com/apache/nuttx/pull/15461#issuecomment-2576675863
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR summary does not adequately meet the NuttX requirements. Here's a breakdown and suggestions for improvement: **Weaknesses:** * **Summary is too brief and technical.** It assumes the reader understands KASAN and MTE. It doesn't explain *why* this change is necessary. What problem does it solve? What benefits does it bring? What are the goals? * **Functional change description is unclear.** "call arm64_mte to implement tagging" is vague. What does tagging mean in this context? How does it work at a high level? * **Log dump is excessive and unhelpful.** The massive log dump doesn't highlight the relevant changes. It shows a crash, which isn't a good demonstration of functionality. It should show specific examples of KASAN catching memory errors. * **Impact section is insufficient.** Simply referencing other PRs doesn't explain the impact of *this* PR. Each PR should stand on its own. The impact on users, build, hardware, documentation, security, and compatibility should be explicitly stated. * **Testing section lacks detail on the host system.** It only mentions the target. What OS, compiler version, and tools were used on the host build machine? * **No "before" and "after" comparison.** The testing logs are all "after." There's no demonstration of how behavior changed due to the PR. Ideally, the logs should show a memory error being detected that wasn't detected before. **Suggestions for Improvement:** * **Expand the Summary:** * Explain the purpose of KASAN and MTE in simple terms. Why is memory protection important? * State the specific goal of this PR. Is it to enable KASAN support? To improve existing KASAN functionality? * Summarize the implementation approach at a high level. * **Clarify the Functional Change:** * Describe how `mte_tags.c` interacts with `arm64_mte`. * Explain the tagging mechanism. What gets tagged, and how are tags used for error detection? * **Improve the Testing Section and Logs:** * Provide host build details (OS, compiler, etc.). * Show "before" logs where a memory error goes undetected (if possible). * Show "after" logs where the same error is caught by KASAN. * Focus on the relevant log entries. Trim the excessive output. * Add a test case specifically designed to trigger and catch a memory error. * **Complete the Impact Section:** * Explain the impact independently of other PRs. Even if it depends on other changes, describe how *this* PR affects the system. * Address all the impact points explicitly (user, build, hardware, documentation, security, compatibility). Use "NO" or "YES" followed by a brief explanation. Don't leave them blank. **Example of an Improved Summary:** This PR implements KASAN (Kernel Address Sanitizer) memory protection for ARM64 hardware using Memory Tagging Extension (MTE). KASAN helps detect memory errors like out-of-bounds accesses and use-after-free bugs. MTE provides hardware support for tagging memory allocations, enabling efficient error detection. This implementation adds `mte_tags.c`, which utilizes the `arm64_mte` functions to tag memory blocks during allocation and deallocation. When a memory access occurs, the hardware compares the access tag with the memory tag. A mismatch indicates a memory error, triggering a fault. This PR enables KASAN by default when `MM_KASAN_MTE_TAGS` is defined for ARM64 MTE-enabled platforms. By making these improvements, the PR will be much clearer, easier to review, and more likely to be accepted. Remember to follow the contributing guidelines and provide a concise yet comprehensive description of your changes. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
