[PATCH] virtio/vsock: fix the transport to work with VMADDR_CID_ANY

[PATCH] virtio/vsock: fix the transport to work with VMADDR_CID_ANY

[Wei Wang]

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 <wei.w.wang@intel.com>
---
 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

RE: [PATCH] virtio/vsock: fix the transport to work with VMADDR_CID_ANY

[Wang, Wei W]

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

Re: [PATCH] virtio/vsock: fix the transport to work with VMADDR_CID_ANY

[Stefano Garzarella]

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

Re: [PATCH] virtio/vsock: fix the transport to work with VMADDR_CID_ANY

[kernel test robot]

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/202111260614.IaGWVZYm-lkp@intel.com/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 <lkp@intel.com>

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/kbuild-all@lists.01.org

RE: [PATCH] virtio/vsock: fix the transport to work with VMADDR_CID_ANY

[Wang, Wei W]

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