Re: [PATCH] usbip: fix vhci races in connection tear down

On 3/12/21 12:08 AM, Hillf Danton wrote:

On Thu, 11 Mar 2021 19:27:37 -0700  Shuah Khan wrote:

quoted

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?

quoted

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