Re: [PATCH net 1/2] vsock/virtio: discard packets if the transport changes

[Stefano Garzarella]

On Thu, Jan 09, 2025 at 02:34:28PM +0100, Michal Luczaj wrote:
> On 1/8/25 19:06, Stefano Garzarella wrote:
> > If the socket has been de-assigned or assigned to another transport,
> > we must discard any packets received because they are not expected
> > and would cause issues when we access vsk->transport.
> >
> > A possible scenario is described by Hyunwoo Kim in the attached link,
> > where after a first connect() interrupted by a signal, and a second
> > connect() failed, we can find `vsk->transport` at NULL, leading to a
> > NULL pointer dereference.
> >
> > Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
> > Reported-by: Hyunwoo Kim <v4bel@xxxxxxxxx>
> > Reported-by: Wongi Lee <qwerty@xxxxxxxxx>
> > Closes: https://lore.kernel.org/netdev/Z2LvdTTQR7dBmPb5@v4bel-B760M-AORUS-ELITE-AX/
> > Signed-off-by: Stefano Garzarella <sgarzare@xxxxxxxxxx>
> > ---
> >  net/vmw_vsock/virtio_transport_common.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> > index 9acc13ab3f82..51a494b69be8 100644
> > --- a/net/vmw_vsock/virtio_transport_common.c
> > +++ b/net/vmw_vsock/virtio_transport_common.c
> > @@ -1628,8 +1628,11 @@ void virtio_transport_recv_pkt(struct virtio_transport *t,
> >
> >  	lock_sock(sk);
> >
> > -	/* Check if sk has been closed before lock_sock */
> > -	if (sock_flag(sk, SOCK_DONE)) {
> > +	/* Check if sk has been closed or assigned to another transport before
> > +	 * lock_sock (note: listener sockets are not assigned to any transport)
> > +	 */
> > +	if (sock_flag(sk, SOCK_DONE) ||
> > +	    (sk->sk_state != TCP_LISTEN && vsk->transport != &t->transport)) {
> >  		(void)virtio_transport_reset_no_sock(t, skb);
> >  		release_sock(sk);
> >  		sock_put(sk);
>
> FWIW, I've tried simplifying Hyunwoo's repro to toy with some tests. Ended
> up with
>
> ```
> from threading import *
> from socket import *
> from signal import *
>
> def listener(tid):
> 	while True:
> 		s = socket(AF_VSOCK, SOCK_SEQPACKET)
> 		s.bind((1, 1234))
> 		s.listen()
> 		pthread_kill(tid, SIGUSR1)
>
> signal(SIGUSR1, lambda *args: None)
> Thread(target=listener, args=[get_ident()]).start()
>
> while True:
> 	c = socket(AF_VSOCK, SOCK_SEQPACKET)
> 	c.connect_ex((1, 1234))
> 	c.connect_ex((42, 1234))
> ```
>
> which gives me splats with or without this patch.

Thanks again for the repro, it worked for me, only if a G2H transport
wasn't loaeded (e.g. virtio-vsock driver).

I tested on the v2 I just sent [1], and I can't see the kernel oops
anymore, but please do test/review too.

Thanks,
Stefano

[1] https://lore.kernel.org/netdev/20250110083511.30419-1-sgarzare@xxxxxxxxxx/