The VMADDR_CID_ANY flag used by a socket means that the socket isn't bound
to any specific CID. For example, a host vsock server may want to be bound
with VMADDR_CID_ANY, so that a guest vsock client can connect to the host
server with CID=VMADDR_CID_HOST (i.e. 2), and meanwhile, a host vsock
client can connect to the same local server with CID=VMADDR_CID_LOCAL
(i.e. 1).
The current implementation sets the destination socket's svm_cid to a
fixed CID value after the first client's connection, which isn't an
expected operation. For example, if the guest client first connects to the
host server, the server's svm_cid gets set to VMADDR_CID_HOST, then other
host clients won't be able to connect to the server anymore.
Reproduce steps:
1. Run the host server:
socat VSOCK-LISTEN:1234,fork -
2. Run a guest client to connect to the host server:
socat - VSOCK-CONNECT:2:1234
3. Run a host client to connect to the host server:
socat - VSOCK-CONNECT:1:1234
Without this patch, step 3. above fails to connect, and socat complains
"socat[1720] E connect(5, AF=40 cid:1 port:1234, 16): Connection
reset by peer".
With this patch, the above works well.
Signed-off-by: Wei Wang <[email protected]>
---
net/vmw_vsock/virtio_transport_common.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 59ee1be5a6dd..5c60fae10569 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -1298,9 +1298,6 @@ void virtio_transport_recv_pkt(struct virtio_transport *t,
space_available = virtio_transport_space_update(sk, pkt);
- /* Update CID in case it has changed after a transport reset event */
- vsk->local_addr.svm_cid = dst.svm_cid;
-
if (space_available)
sk->sk_write_space(sk);
--
2.25.1
On Thursday, November 25, 2021 3:16 PM, Wang, Wei W wrote:
> - /* Update CID in case it has changed after a transport reset event */
> - vsk->local_addr.svm_cid = dst.svm_cid;
> -
> if (space_available)
> sk->sk_write_space(sk);
>
Not sure if anybody knows how this affects the transport reset.
Thanks,
Wei
On Thu, Nov 25, 2021 at 09:27:40AM +0000, Wang, Wei W wrote:
>On Thursday, November 25, 2021 3:16 PM, Wang, Wei W wrote:
>> - /* Update CID in case it has changed after a transport reset event */
>> - vsk->local_addr.svm_cid = dst.svm_cid;
>> -
>> if (space_available)
>> sk->sk_write_space(sk);
>>
>
>Not sure if anybody knows how this affects the transport reset.
I believe the primary use case is when a guest is migrated.
After the migration, the transport gets a reset event from the
hypervisor and all connected sockets are closed. The ones in listen
remain open though.
Also the guest's CID may have changed after migration. So if an
application has open listening sockets, bound to the old CID, this
should ensure that the socket continues to be usable.
The patch would then change this behavior.
So maybe to avoid problems, we could update the CID only if it is
different from VMADDR_CID_ANY:
if (vsk->local_addr.svm_cid != VMADDR_CID_ANY)
vsk->local_addr.svm_cid = dst.svm_cid;
When this code was written, a guest only supported a single transport,
so it could only have one CID assigned, so that wasn't a problem.
For that reason I'll add this Fixes tag:
Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
Thanks,
Stefano
Hi Wei,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on mst-vhost/linux-next]
[also build test WARNING on net-next/master net/master linus/master v5.16-rc2 next-20211125]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Wei-Wang/virtio-vsock-fix-the-transport-to-work-with-VMADDR_CID_ANY/20211125-163238
base: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
config: riscv-allyesconfig (https://download.01.org/0day-ci/archive/20211126/[email protected]/config)
compiler: riscv64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/007dbd2e6e604bf8b17a4cec1357113a26983838
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Wei-Wang/virtio-vsock-fix-the-transport-to-work-with-VMADDR_CID_ANY/20211125-163238
git checkout 007dbd2e6e604bf8b17a4cec1357113a26983838
# save the config file to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=riscv
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
net/vmw_vsock/virtio_transport_common.c: In function 'virtio_transport_recv_pkt':
>> net/vmw_vsock/virtio_transport_common.c:1246:28: warning: variable 'vsk' set but not used [-Wunused-but-set-variable]
1246 | struct vsock_sock *vsk;
| ^~~
vim +/vsk +1246 net/vmw_vsock/virtio_transport_common.c
e4b1ef152f53d5e Arseny Krasnov 2021-06-11 1238
06a8fc78367d070 Asias He 2016-07-28 1239 /* We are under the virtio-vsock's vsock->rx_lock or vhost-vsock's vq->mutex
06a8fc78367d070 Asias He 2016-07-28 1240 * lock.
06a8fc78367d070 Asias He 2016-07-28 1241 */
4c7246dc45e2706 Stefano Garzarella 2019-11-14 1242 void virtio_transport_recv_pkt(struct virtio_transport *t,
4c7246dc45e2706 Stefano Garzarella 2019-11-14 1243 struct virtio_vsock_pkt *pkt)
06a8fc78367d070 Asias He 2016-07-28 1244 {
06a8fc78367d070 Asias He 2016-07-28 1245 struct sockaddr_vm src, dst;
06a8fc78367d070 Asias He 2016-07-28 @1246 struct vsock_sock *vsk;
06a8fc78367d070 Asias He 2016-07-28 1247 struct sock *sk;
06a8fc78367d070 Asias He 2016-07-28 1248 bool space_available;
06a8fc78367d070 Asias He 2016-07-28 1249
f83f12d660d1171 Michael S. Tsirkin 2016-12-06 1250 vsock_addr_init(&src, le64_to_cpu(pkt->hdr.src_cid),
06a8fc78367d070 Asias He 2016-07-28 1251 le32_to_cpu(pkt->hdr.src_port));
f83f12d660d1171 Michael S. Tsirkin 2016-12-06 1252 vsock_addr_init(&dst, le64_to_cpu(pkt->hdr.dst_cid),
06a8fc78367d070 Asias He 2016-07-28 1253 le32_to_cpu(pkt->hdr.dst_port));
06a8fc78367d070 Asias He 2016-07-28 1254
06a8fc78367d070 Asias He 2016-07-28 1255 trace_virtio_transport_recv_pkt(src.svm_cid, src.svm_port,
06a8fc78367d070 Asias He 2016-07-28 1256 dst.svm_cid, dst.svm_port,
06a8fc78367d070 Asias He 2016-07-28 1257 le32_to_cpu(pkt->hdr.len),
06a8fc78367d070 Asias He 2016-07-28 1258 le16_to_cpu(pkt->hdr.type),
06a8fc78367d070 Asias He 2016-07-28 1259 le16_to_cpu(pkt->hdr.op),
06a8fc78367d070 Asias He 2016-07-28 1260 le32_to_cpu(pkt->hdr.flags),
06a8fc78367d070 Asias He 2016-07-28 1261 le32_to_cpu(pkt->hdr.buf_alloc),
06a8fc78367d070 Asias He 2016-07-28 1262 le32_to_cpu(pkt->hdr.fwd_cnt));
06a8fc78367d070 Asias He 2016-07-28 1263
e4b1ef152f53d5e Arseny Krasnov 2021-06-11 1264 if (!virtio_transport_valid_type(le16_to_cpu(pkt->hdr.type))) {
4c7246dc45e2706 Stefano Garzarella 2019-11-14 1265 (void)virtio_transport_reset_no_sock(t, pkt);
06a8fc78367d070 Asias He 2016-07-28 1266 goto free_pkt;
06a8fc78367d070 Asias He 2016-07-28 1267 }
06a8fc78367d070 Asias He 2016-07-28 1268
06a8fc78367d070 Asias He 2016-07-28 1269 /* The socket must be in connected or bound table
06a8fc78367d070 Asias He 2016-07-28 1270 * otherwise send reset back
06a8fc78367d070 Asias He 2016-07-28 1271 */
06a8fc78367d070 Asias He 2016-07-28 1272 sk = vsock_find_connected_socket(&src, &dst);
06a8fc78367d070 Asias He 2016-07-28 1273 if (!sk) {
06a8fc78367d070 Asias He 2016-07-28 1274 sk = vsock_find_bound_socket(&dst);
06a8fc78367d070 Asias He 2016-07-28 1275 if (!sk) {
4c7246dc45e2706 Stefano Garzarella 2019-11-14 1276 (void)virtio_transport_reset_no_sock(t, pkt);
06a8fc78367d070 Asias He 2016-07-28 1277 goto free_pkt;
06a8fc78367d070 Asias He 2016-07-28 1278 }
06a8fc78367d070 Asias He 2016-07-28 1279 }
06a8fc78367d070 Asias He 2016-07-28 1280
e4b1ef152f53d5e Arseny Krasnov 2021-06-11 1281 if (virtio_transport_get_type(sk) != le16_to_cpu(pkt->hdr.type)) {
e4b1ef152f53d5e Arseny Krasnov 2021-06-11 1282 (void)virtio_transport_reset_no_sock(t, pkt);
e4b1ef152f53d5e Arseny Krasnov 2021-06-11 1283 sock_put(sk);
e4b1ef152f53d5e Arseny Krasnov 2021-06-11 1284 goto free_pkt;
e4b1ef152f53d5e Arseny Krasnov 2021-06-11 1285 }
e4b1ef152f53d5e Arseny Krasnov 2021-06-11 1286
06a8fc78367d070 Asias He 2016-07-28 1287 vsk = vsock_sk(sk);
06a8fc78367d070 Asias He 2016-07-28 1288
06a8fc78367d070 Asias He 2016-07-28 1289 lock_sock(sk);
06a8fc78367d070 Asias He 2016-07-28 1290
3fe356d58efae54 Stefano Garzarella 2020-11-20 1291 /* Check if sk has been closed before lock_sock */
3fe356d58efae54 Stefano Garzarella 2020-11-20 1292 if (sock_flag(sk, SOCK_DONE)) {
8692cefc433f282 Jia He 2020-05-30 1293 (void)virtio_transport_reset_no_sock(t, pkt);
8692cefc433f282 Jia He 2020-05-30 1294 release_sock(sk);
8692cefc433f282 Jia He 2020-05-30 1295 sock_put(sk);
8692cefc433f282 Jia He 2020-05-30 1296 goto free_pkt;
8692cefc433f282 Jia He 2020-05-30 1297 }
8692cefc433f282 Jia He 2020-05-30 1298
ce7536bc7398e2a Stefano Garzarella 2021-02-08 1299 space_available = virtio_transport_space_update(sk, pkt);
ce7536bc7398e2a Stefano Garzarella 2021-02-08 1300
06a8fc78367d070 Asias He 2016-07-28 1301 if (space_available)
06a8fc78367d070 Asias He 2016-07-28 1302 sk->sk_write_space(sk);
06a8fc78367d070 Asias He 2016-07-28 1303
06a8fc78367d070 Asias He 2016-07-28 1304 switch (sk->sk_state) {
3b4477d2dcf2709 Stefan Hajnoczi 2017-10-05 1305 case TCP_LISTEN:
c0cfa2d8a788fcf Stefano Garzarella 2019-11-14 1306 virtio_transport_recv_listen(sk, pkt, t);
06a8fc78367d070 Asias He 2016-07-28 1307 virtio_transport_free_pkt(pkt);
06a8fc78367d070 Asias He 2016-07-28 1308 break;
3b4477d2dcf2709 Stefan Hajnoczi 2017-10-05 1309 case TCP_SYN_SENT:
06a8fc78367d070 Asias He 2016-07-28 1310 virtio_transport_recv_connecting(sk, pkt);
06a8fc78367d070 Asias He 2016-07-28 1311 virtio_transport_free_pkt(pkt);
06a8fc78367d070 Asias He 2016-07-28 1312 break;
3b4477d2dcf2709 Stefan Hajnoczi 2017-10-05 1313 case TCP_ESTABLISHED:
06a8fc78367d070 Asias He 2016-07-28 1314 virtio_transport_recv_connected(sk, pkt);
06a8fc78367d070 Asias He 2016-07-28 1315 break;
3b4477d2dcf2709 Stefan Hajnoczi 2017-10-05 1316 case TCP_CLOSING:
06a8fc78367d070 Asias He 2016-07-28 1317 virtio_transport_recv_disconnecting(sk, pkt);
06a8fc78367d070 Asias He 2016-07-28 1318 virtio_transport_free_pkt(pkt);
06a8fc78367d070 Asias He 2016-07-28 1319 break;
06a8fc78367d070 Asias He 2016-07-28 1320 default:
df12eb6d6cd920a Sebastien Boeuf 2020-02-14 1321 (void)virtio_transport_reset_no_sock(t, pkt);
06a8fc78367d070 Asias He 2016-07-28 1322 virtio_transport_free_pkt(pkt);
06a8fc78367d070 Asias He 2016-07-28 1323 break;
06a8fc78367d070 Asias He 2016-07-28 1324 }
c0cfa2d8a788fcf Stefano Garzarella 2019-11-14 1325
06a8fc78367d070 Asias He 2016-07-28 1326 release_sock(sk);
06a8fc78367d070 Asias He 2016-07-28 1327
06a8fc78367d070 Asias He 2016-07-28 1328 /* Release refcnt obtained when we fetched this socket out of the
06a8fc78367d070 Asias He 2016-07-28 1329 * bound or connected list.
06a8fc78367d070 Asias He 2016-07-28 1330 */
06a8fc78367d070 Asias He 2016-07-28 1331 sock_put(sk);
06a8fc78367d070 Asias He 2016-07-28 1332 return;
06a8fc78367d070 Asias He 2016-07-28 1333
06a8fc78367d070 Asias He 2016-07-28 1334 free_pkt:
06a8fc78367d070 Asias He 2016-07-28 1335 virtio_transport_free_pkt(pkt);
06a8fc78367d070 Asias He 2016-07-28 1336 }
06a8fc78367d070 Asias He 2016-07-28 1337 EXPORT_SYMBOL_GPL(virtio_transport_recv_pkt);
06a8fc78367d070 Asias He 2016-07-28 1338
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Thursday, November 25, 2021 6:41 PM, Stefano Garzarella wrote:
> On Thu, Nov 25, 2021 at 09:27:40AM +0000, Wang, Wei W wrote:
> >On Thursday, November 25, 2021 3:16 PM, Wang, Wei W wrote:
> >> - /* Update CID in case it has changed after a transport reset event */
> >> - vsk->local_addr.svm_cid = dst.svm_cid;
> >> -
> >> if (space_available)
> >> sk->sk_write_space(sk);
> >>
> >
> >Not sure if anybody knows how this affects the transport reset.
>
> I believe the primary use case is when a guest is migrated.
>
> After the migration, the transport gets a reset event from the hypervisor and
> all connected sockets are closed. The ones in listen remain open though.
>
> Also the guest's CID may have changed after migration. So if an application has
> open listening sockets, bound to the old CID, this should ensure that the socket
> continues to be usable.
OK, thanks for sharing the background.
>
> The patch would then change this behavior.
>
> So maybe to avoid problems, we could update the CID only if it is different
> from VMADDR_CID_ANY:
>
> if (vsk->local_addr.svm_cid != VMADDR_CID_ANY)
> vsk->local_addr.svm_cid = dst.svm_cid;
>
>
> When this code was written, a guest only supported a single transport, so it
> could only have one CID assigned, so that wasn't a problem.
> For that reason I'll add this Fixes tag:
> Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
Sounds good to me.
Thanks,
Wei