netdev - Re: [PATCH net v2 1/5] vsock/virtio: discard packets if the transport changes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7de34054-10cf-45d0-a869-adebb77ad913@rbox.co>
Date: Tue, 14 Jan 2025 01:09:24 +0100
From: Michal Luczaj <mhal@...x.co>
To: Stefano Garzarella <sgarzare@...hat.com>
Cc: netdev@...r.kernel.org, Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
 bpf@...r.kernel.org, linux-kernel@...r.kernel.org,
 Luigi Leonardi <leonardi@...hat.com>, "David S. Miller"
 <davem@...emloft.net>, Wongi Lee <qwerty@...ori.io>,
 Eugenio Pérez <eperezma@...hat.com>,
 "Michael S. Tsirkin" <mst@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
 kvm@...r.kernel.org, Paolo Abeni <pabeni@...hat.com>,
 Stefan Hajnoczi <stefanha@...hat.com>, Jason Wang <jasowang@...hat.com>,
 Simon Horman <horms@...nel.org>, Hyunwoo Kim <v4bel@...ori.io>,
 Jakub Kicinski <kuba@...nel.org>, virtualization@...ts.linux.dev,
 stable@...r.kernel.org
Subject: Re: [PATCH net v2 1/5] vsock/virtio: discard packets if the transport
 changes

On 1/13/25 16:01, Stefano Garzarella wrote:
> On Mon, Jan 13, 2025 at 02:51:58PM +0100, Michal Luczaj wrote:
>> On 1/13/25 12:05, Stefano Garzarella wrote:
>>> ...
>>> An alternative approach, which would perhaps allow us to avoid all this,
>>> is to re-insert the socket in the unbound list after calling release()
>>> when we deassign the transport.
>>>
>>> WDYT?
>>
>> If we can't keep the old state (sk_state, transport, etc) on failed
>> re-connect() then reverting back to initial state sounds, uhh, like an
>> option :) I'm not sure how well this aligns with (user's expectations of)
>> good ol' socket API, but maybe that train has already left.
> 
> We really want to behave as similar as possible with the other sockets,
> like AF_INET, so I would try to continue toward that train.

I was worried that such connect()/transport error handling may have some
user visible side effects, but I guess I was wrong. I mean you can still
reach a sk_state=TCP_LISTEN with a transport assigned[1], but perhaps
that's a different issue.

I've tried your suggestion on top of this series. Passes the tests.

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index fa9d1b49599b..4718fe86689d 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -492,6 +492,10 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
 		vsk->transport->release(vsk);
 		vsock_deassign_transport(vsk);

+		vsock_addr_unbind(&vsk->local_addr);
+		vsock_addr_unbind(&vsk->remote_addr);
+		vsock_insert_unbound(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

>> Another possibility would be to simply brick the socket on failed (re)connect.
> 
> I see, though, this is not the behavior of AF_INET for example, right?

Right.

> Do you have time to investigate/fix this problem?
> If not, I'll try to look into it in the next few days, maybe next week.

I'm happy to help, but it's not like I have any better ideas.

Michal

[1]: E.g. this way:
```
from socket import *

MAX_PORT_RETRIES = 24 # net/vmw_vsock/af_vsock.c
VMADDR_CID_LOCAL = 1
VMADDR_PORT_ANY = -1
hold = []

def take_port(port):
	s = socket(AF_VSOCK, SOCK_SEQPACKET)
	s.bind((VMADDR_CID_LOCAL, port))
	hold.append(s)
	return s

s = take_port(VMADDR_PORT_ANY)
_, port = s.getsockname()
for _ in range(MAX_PORT_RETRIES):
	port += 1
	take_port(port);

s = socket(AF_VSOCK, SOCK_SEQPACKET)
err = s.connect_ex((VMADDR_CID_LOCAL, port))
assert err != 0
print("ok, connect failed; transport set")

s.bind((VMADDR_CID_LOCAL, port+1))
s.listen(16)
```