Messages in this thread | | | Subject | Re: [PATCH] usbip: fix vhci races in connection tear down | From | Shuah Khan <> | Date | Fri, 12 Mar 2021 13:42:26 -0700 |
| |
On 3/12/21 12:08 AM, Hillf Danton wrote: > On Thu, 11 Mar 2021 19:27:37 -0700 Shuah Khan wrote: >> vhci_shutdown_connection() references connection state (tcp_socket, >> tcp_rx, tcp_tx, sockfd) saved in usbpip_device without holding the >> lock. >> >> Current connection tear down sequence: >> Step 1: shutdown the socket >> Step 2: stop rx thread and reset tcp_rx pointer >> Step 3: stop tx thread and reset tcp_tx pointer >> Step 4: Reset tcp_socket and sockfd >> >> There are several race windows between these steps. In addition, device >> reset routine (vhci_device_reset) resets tcp_socket and sockfd holding >> the lock. > > Can you specify the scenario where reset runs in race with teardown as > both are parts of usbip_work on a singlethread workqueue? >>
Hmm. I can't think of one. I was concerned about any async paths that potentially interfere with shutdown. With vhci_shutdown_connection() being so relaxed with locking, this is a cautious approach on my part. I am also keeping in mind that this problem shows up in a limited scope fuzzing test that doesn't trigger any other normal paths that would be active if there is real device on the other side.
As for the tcp_socket check in the reset routine, I am not positive what purpose it serves. I introduced the in_disconnect flag so shutdown and reset don't collide, in case I am missing some scenario in the normal path when we actually have a actual device attached.
With the other locking and error path problems in addressed, both shutdown and reset could be made simpler.
In any case, I think in_disconnect might be too big a hammer. I will redo the patch without it and also remove tcp_socket handling from the reset routine. I don't see USBIP_EH_RESET getting set without USBIP_EH_SHUTDOWN.
thanks, -- Shuah
| |