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

[Shuah Khan]

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