vsock: Implement sibling VM communication

[techiepriyansh]

Adds support for communication between sibling VMs that use the vhost-user-vsock devices from the same vhost-user-vsock application.

[stefano-garzarella]

Good job!

I left several minor comments. I recommend that you add an example of how you tested the patch in the commit message.

Another thing I think we should put in is something to decide whether 2 VMs should communicate or not.
One idea would be to add a new "group" parameter to each VM and have only the VMs in the same "group" communicate.

Obviously we can do that later, but I wanted to tell you before I forget.

[stefano-garzarella]

Ah I think we should extend the README and say if you add more VMs they can communicate with each other.

Another thing I have to check now that we have more VMs, is that the packets received from a VM have the right src_cid, otherwise one might impersonate another.

I think we didn't have this check before, but now we should since we have more VMs.

[techiepriyansh]

I recommend that you add an example of how you tested the patch in the commit message.

For testing, I used https://github.com/stefanha/nc-vsock/blob/master/nc-vsock.c with this patch:

diff --git a/nc-vsock.c b/nc-vsock.c
index 1704151..beda0fc 100644
--- a/nc-vsock.c
+++ b/nc-vsock.c
@@ -146,6 +146,7 @@ static int vsock_connect(const char *cid_str, const char *port_str)
 	int port;
 	struct sockaddr_vm sa = {
 		.svm_family = AF_VSOCK,
+		.svm_flags = VMADDR_FLAG_TO_HOST,
 	};
 
 	cid = parse_cid(cid_str);

Should I mention all this in the commit message, or should I make a fork with this patch applied and then link to it?

[stefano-garzarella]

I recommend that you add an example of how you tested the patch in the commit message.

Should I mention all this in the commit message, or should I make a fork with this patch applied and then link to it?

Should be fine something like:

Tested with nc-vsock patched to set `.svm_flags = VMADDR_FLAG_TO_HOST`:
  host$ ./vhost-user-vsock .....
  
  vm1$ nc-vsock ...
  vm2$ nc-vsock ...

[techiepriyansh]

I am getting a cargo audit error in the checks. Should I commit the rebuilt cargo lock file by running cargo generate-lockfile in this PR itself?

[stefano-garzarella]

I am getting a cargo audit error in the checks. Should I commit the rebuilt cargo lock file by running cargo generate-lockfile in this PR itself?

Do you have the same issue on the main branch?

If it is the case, will be better to have another PR.
Instead, if it is related to these changes, it is fine to do in this PR.

[techiepriyansh]

Do you have the same issue on the main branch?

Yes

If it is the case, will be better to have another PR.

Okay

[techiepriyansh]

I've created the PR fixing cargo audit errors over at #371.

[techiepriyansh]

Rebased over main now that #371 has been merged.

[stefano-garzarella]

@techiepriyansh the final result LGTM!

The only thing I'd suggest is to reorganize the commits, I'd move the new 2 commits, before the one that adds support for communication between multiple VMs, so we don't add code (e.g. cid_map) that we edit right after that.

[techiepriyansh]

I've now moved the commit handling the single VM case same as multiple VMs before the commit implementing sibling VM communication. I've not moved the commit checking the packet source CID yet because I think it makes more sense to put it afterwards, but do let me know if I need to move it back too.

[stefano-garzarella]

I've now moved the commit handling the single VM case same as multiple VMs before the commit implementing sibling VM communication. I've not moved the commit checking the packet source CID yet because I think it makes more sense to put it afterwards, but do let me know if I need to move it back too.

LGTM!