You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "jlaitine (via GitHub)" <gi...@apache.org> on 2024/11/06 12:59:13 UTC
[PR] Fix poll notify [nuttx]
jlaitine opened a new pull request, #14670:
URL: https://github.com/apache/nuttx/pull/14670
## Summary
Fix a race condition in poll_notify, revert seemingly unnecessary change in serial driver.
Originally, the race condition was found with an older nuttx version inside the poll_notify, and the same race condition exists also in the current upstream version.
This PR has the first patch, which protects from the RMW issue. Second patch is an optimization to the poll_notify, but also fixes another race which may cause callback being called twice.
For the serial driver, the issue has been circumvented in #13914, but the race condition should be fixed in poll_notify, as it exists for all the poll interfaces, and not just serial driver. I *believe* that the serial driver fix is unnecessary, so third patch is reverting that - it is not nice to add sched_locks around the drivers unnecessarily.
## Impact
Impacts the implementation of poll / poll_notify, fixing a read-modify-write race condition.
## Testing
The code has been inspected by 2 engineers, and desk-tested with real application using the poll interface. However, I don't have a possibility to run extensive testing which would show the exact issue which was fixed in #13914, so I'd appreciate if the solution is reviewed & tested by e.g. @CV-Bowen who fixed the race condition for serial driver.
--
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
Re: [PR] Fix poll notify [nuttx]
Posted by "jlaitine (via GitHub)" <gi...@apache.org>.
jlaitine commented on PR #14670:
URL: https://github.com/apache/nuttx/pull/14670#issuecomment-2463925341
re-based to current master
--
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
Re: [PR] Fix poll notify [nuttx]
Posted by "nuttxpr (via GitHub)" <gi...@apache.org>.
nuttxpr commented on PR #14670:
URL: https://github.com/apache/nuttx/pull/14670#issuecomment-2459715738
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues)
Yes, this PR mostly meets the NuttX requirements, although some clarifications and additions are needed.
**Missing/Needs Improvement:**
* **Summary:** While the summary explains the *what*, it lacks detail on *how* the changes fix the race condition. Describe the specific mechanism used (e.g., mutex, atomic operation, etc.) in each patch. Briefly mention *why* the serial driver change in #13914 is believed to be unnecessary.
* **Impact:**
* **User Impact:** While it fixes a bug, does this introduce any behavioral changes the user might observe? If the answer is truly NO, explicitly state "NO". If there are subtle timing changes that *could* affect user code (even if unlikely), mention that.
* **Build Impact:** NO (Explicitly state)
* **Hardware Impact:** NO (Explicitly state)
* **Documentation Impact:** NO (Explicitly state, or YES if documentation needs updating).
* **Security Impact:** This fixes a race condition. This likely has positive security implications. Describe this briefly. E.g., "YES - Improves robustness and potentially mitigates denial-of-service possibilities due to the race condition."
* **Compatibility Impact:** NO (Explicitly state, unless there's a subtle edge case)
* **Testing:** The testing section is weak. Desk testing and code inspection are good, but insufficient for a race condition fix. While you may not be able to reproduce the original issue, you should attempt to create a test case that stresses the `poll_notify` mechanism and demonstrates that the race condition is no longer present. Even a simple test case is better than none. Explain *why* you can't fully reproduce the original issue if possible. Reaching out to @CV-Bowen for review and testing is good practice.
**Recommendations:**
1. **Elaborate on the "how" in the Summary.** Provide a concise, technical description of the fix for each patch.
2. **Complete all Impact sections explicitly.** Even if the answer is "NO", state it clearly.
3. **Strengthen the Testing section.** Create at least one test case, even if it's a simple one, to demonstrate the absence of the race condition after your changes. Document the limitations of your testing environment and explain why reproducing the original issue is difficult.
By addressing these points, you will significantly improve the quality of your PR and make it easier for reviewers to understand and accept 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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix poll notify [nuttx]
Posted by "jlaitine (via GitHub)" <gi...@apache.org>.
jlaitine closed pull request #14670: Fix poll notify
URL: https://github.com/apache/nuttx/pull/14670
--
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