Re: [PATCH RFC net v2 3/3] vsock: Fix IOCTL_VM_SOCKETS_GET_LOCAL_CID to check also `transport_local`

[Stefano Garzarella]

On Wed, Jun 25, 2025 at 11:23:54PM +0200, Michal Luczaj wrote:
> On 6/25/25 10:54, Stefano Garzarella wrote:
> > On Fri, Jun 20, 2025 at 09:52:45PM +0200, Michal Luczaj wrote:
> > > Support returning VMADDR_CID_LOCAL in case no other vsock transport is
> > > available.
> > >
> > > Fixes: 0e12190578d0 ("vsock: add local transport support in the vsock core")
> > > Suggested-by: Stefano Garzarella <sgarzare@xxxxxxxxxx>
> > > Signed-off-by: Michal Luczaj <mhal@xxxxxxx>
> > > ---
> > > man vsock(7) mentions IOCTL_VM_SOCKETS_GET_LOCAL_CID vs. VMADDR_CID_LOCAL:
> > >
> > >   Ioctls
> > >       ...
> > >       IOCTL_VM_SOCKETS_GET_LOCAL_CID
> > >              ...
> > >              Consider using VMADDR_CID_ANY when binding instead of
> > >              getting the local CID with IOCTL_VM_SOCKETS_GET_LOCAL_CID.
> > >
> > >   Local communication
> > >       ....
> > >       The local CID obtained with IOCTL_VM_SOCKETS_GET_LOCAL_CID can be
> > >       used for the same purpose, but it is preferable to use
> > >       VMADDR_CID_LOCAL.
> > >
> > > I was wondering it that would need some rewriting, since we're adding
> > > VMADDR_CID_LOCAL as a possible ioctl's return value.
> >
> > IIRC the reason was, that if we have for example a G2H module loaded,
> > the ioctl returns the CID of that module (e.g. 42). So, we can use both
> > 42 and VMADDR_CID_LOCAL to do the loopback communication, but we
> > encourage to always use VMADDR_CID_LOCAL.  With this change we basically
> > don't change that, but we change the fact that if there is only the
> > loopback module loaded, before the ioctl returned VMADDR_CID_ANY, while
> > now it returns LOCAL rightly.
> >
> > So, IMO we are fine.
>
> All right, got it.
>
> > > ---
> > > net/vmw_vsock/af_vsock.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> > > index a1b1073a2c89f865fcdb58b38d8e7feffcf1544f..4bdb4016bd14d790f3d217d5063be64a1553b194 100644
> > > --- a/net/vmw_vsock/af_vsock.c
> > > +++ b/net/vmw_vsock/af_vsock.c
> > > @@ -2577,6 +2577,8 @@ static long vsock_dev_do_ioctl(struct file *filp,
> > > 		cid = vsock_transport_local_cid(&transport_g2h);
> > > 		if (cid == VMADDR_CID_ANY)
> > > 			cid = vsock_transport_local_cid(&transport_h2g);
> > > +		if (cid == VMADDR_CID_ANY && transport_local)
> > > +			cid = VMADDR_CID_LOCAL;
> >
> > why not `cid = vsock_transport_local_cid(&transport_local)` like for
> > H2G?
>
> Sure, can do. I've assumed transport_local would always have a local CID of
> VMADDR_CID_LOCAL. So taking mutex and going through a callback function to
> get VMADDR_CID_LOCAL seemed superfluous. But I get it, if you want to have
> it symmetrical with the other vsock_transport_local_cid()s.

Yeah, BTW for transport_h2g is the same, they always should return 
VMADDR_CID_HOST, so I think we should be symmetrical.

Thanks,
Stefano