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

drivers/usbdev/cdcacm.c: Fix a crash in cdcacm if usbdev gets unregistered while client calls close for the tty #15485

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

jlaitine
Copy link
Contributor

@jlaitine jlaitine commented Jan 10, 2025

Summary

Make sure that the cdcacm is disconnected before the usbdev gets unregistered.

Also, check if the device is connected or not in cdcuart_txempty (uart_txempty). Otherwise there may be a crash during uart_tcdrain, called in tty close path, if the usbdev unregistration happens during the loop.

This issue can be triggered by monitoring the cable connection status in one thread, sending BOARDIOC_USBDEV_DISCONNECT if the usb cable is detached. In another thread close the ttyACM.

Impact

This fixes a race condition in ttyACM/serial stack, leading to a kernel crash

Testing

Tested with imx9 platform in PX4 stack, where one thread manages the USB connection state and another reads/writes to the ttyACM serial.

…tered while client calls close for the tty

Make sure that the cdcacm is disconnected before the usbdev gets unregistered.

Also, check if the device is connected or not in cdcuart_txempty (uart_txempty). Otherwise there may be a crash during uart_tcdrain, called in tty close path, if the usbdev unregistration happens during the loop.

This issue can be triggered by monitoring the cable connection status in one thread, sending BOARDIOC_USBDEV_DISCONNECT if the usb cable is detached. In another thread close the ttyACM.

Signed-off-by: Jukka Laitinen <[email protected]>
@github-actions github-actions bot added Area: USB Size: S The size of the change in this PR is small labels Jan 10, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 10, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements, although it could be more thorough. Here's a breakdown and suggestions for improvement:

Strengths:

  • Clear Summary: The summary explains the "why" and the "how" of the change.
  • Impact Addresses Key Areas: The impact section identifies the core issue (a crash) and acknowledges the affected area (ttyACM/serial stack).
  • Testing Provides Context: Testing mentions the specific platform and the scenario that triggered the original issue.

Weaknesses and Suggestions for Improvement:

  • Missing Issue References: The summary should link to a related NuttX or NuttX Apps issue if one exists. If not, consider creating one before the PR to document the bug.
  • Impact Lacks Detail: While the impact section mentions a crash, it should be more explicit:
    • User Impact: YES - Users could experience unexpected application termination due to the kernel crash. Specify the conditions under which this might occur (e.g., detaching the USB cable while the serial port is in use).
    • Hardware Impact: While mentioning imx9 in testing is helpful, the Impact section should state explicitly if this is the only affected hardware or if other architectures using cdcacm are also impacted. NO/YES needs to be stated directly.
    • Other Impact Areas: Explicitly state NO for all other impact areas (build, documentation, security, compatibility) or YES if any are relevant. Don't leave them blank.
  • Testing Lacks Logs and Specifics:
    • Build Host: Provide details about the build host OS, CPU architecture, and compiler version.
    • Target: Be more specific. Instead of just "imx9 platform in PX4 stack," specify the board and configuration. e.g., imxrt1064-evk:px4_fmu-v5_default.
    • Logs: The "Testing logs before change" and "Testing logs after change" sections are empty. Include actual log output demonstrating the crash before the fix and the correct behavior after the fix. This is crucial for verifying the fix. Even if logs are long, excerpts highlighting the key differences are necessary.
    • Test Cases: Describe the specific steps taken to test the change. For example:
      1. Connect USB cable.
      2. Open serial port in application.
      3. Detach USB cable.
      4. Observe system behavior (crash before fix, stable operation after fix).

Example of Improved Testing Section:

Testing

I confirm that changes are verified on local setup and works as intended:
* Build Host(s): Linux (Ubuntu 20.04), x86_64, GCC 9.4.0
* Target(s): imxrt1064-evk:px4_fmu-v5_default

Testing logs before change:

[timestamp] app: Opening /dev/ttyACM0
[timestamp] usb: Cable detached
[timestamp] kernel: PANIC!!! cdcacm_interrupt: device not connected


Testing logs after change:

[timestamp] app: Opening /dev/ttyACM0
[timestamp] usb: Cable detached
[timestamp] cdcacm: Device disconnected
[timestamp] app: /dev/ttyACM0 closed


Test Procedure:
1. Built NuttX and PX4 firmware with the described configuration.
2. Flashed the firmware to the imxrt1064-evk board.
3. Connected the board via USB.
4. Opened a serial terminal to /dev/ttyACM0.
5. Ran a script that monitors the USB connection state and sends BOARDIOC_USBDEV_DISCONNECT when the cable is detached.
6. Detached the USB cable.
7. Before the fix, the system panicked.  After the fix, the serial port closed gracefully.


By addressing these points, the PR will be significantly stronger and more likely to be accepted.

@xiaoxiang781216 xiaoxiang781216 merged commit 575c608 into apache:master Jan 10, 2025
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: USB 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