Re: [PATCH net v2 4/5] vsock: reset socket state when de-assigning the transport

[Stefano Garzarella]

On Fri, Jan 10, 2025 at 11:56:28AM +0100, Luigi Leonardi wrote:
> On Fri, Jan 10, 2025 at 09:35:10AM +0100, Stefano Garzarella wrote:
> > Transport's release() and destruct() are called when de-assigning the
> > vsock transport. These callbacks can touch some socket state like
> > sock flags, sk_state, and peer_shutdown.
> >
> > Since we are reassigning the socket to a new transport during
> > vsock_connect(), let's reset these fields to have a clean state with
> > the new transport.
> >
> > Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Stefano Garzarella <sgarzare@xxxxxxxxxx>
> > ---
> > net/vmw_vsock/af_vsock.c | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> > index 5cf8109f672a..74d35a871644 100644
> > --- a/net/vmw_vsock/af_vsock.c
> > +++ b/net/vmw_vsock/af_vsock.c
> > @@ -491,6 +491,15 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
> > 		 */
> > 		vsk->transport->release(vsk);
> > 		vsock_deassign_transport(vsk);
> > +
> > +		/* transport's release() and destruct() can touch some socket
> > +		 * state, since we are reassigning the socket to a new transport
> > +		 * during vsock_connect(), let's reset these fields to have a
> > +		 * clean state.
> > +		 */
> > +		sock_reset_flag(sk, SOCK_DONE);
> > +		sk->sk_state = TCP_CLOSE;
> > +		vsk->peer_shutdown = 0;
> > 	}
> >
> > 	/* We increase the module refcnt to prevent the transport unloading
> > --
> > 2.47.1
>
> Hi Stefano,
> I spent some time investigating what would happen if the scheduled work
> ran before `virtio_transport_cancel_close_work`. IIUC that should do 
> no harm and all the fields are reset correctly.

Yep, after transport->destruct() call, the delayed work should have 
already finished or canceled.

> Thank you,
> Luigi
>
> Reviewed-by: Luigi Leonardi <leonardi@xxxxxxxxxx>

Thanks for the review,
Stefano