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. That said, when I apply this patch, but drop the `sk->sk_state != TCP_LISTEN &&`: no more splats.
We can't drop `sk->sk_state != TCP_LISTEN &&` because listener socket doesn't have any transport (vsk->transport == NULL), so every connection request will receive an error, so maybe this is the reason of no splats.
I'm cooking some more patches to fix Hyunwoo's scenario handling better the close work when the virtio destructor is called.
I'll run your reproduces to test it, thanks for that! Stefano
