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

2021-11-25 Thread Wang, Wei W 
On Thursday, November 25, 2021 6:41 PM, Stefano Garzarella wrote:
> On Thu, Nov 25, 2021 at 09:27:40AM +, 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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

2021-11-25 Thread 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-...@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 

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(, 
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(, 
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

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

2021-11-25 Thread Stefano Garzarella 

On Thu, Nov 25, 2021 at 09:27:40AM +, 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

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

2021-11-25 Thread 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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

2021-11-25 Thread 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 
---
 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

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization