Linux Virtualization - Aug 2023 - [PATCH RFC net-next v5 03/14] af_vsock: support multi-transport datagrams

On Thu, Aug 03, 2023 at 06:58:24PM +0000, Bobby Eshleman
wrote:
>On Thu, Aug 03, 2023 at 02:42:26PM +0200, Stefano Garzarella wrote:
>> On Thu, Aug 03, 2023 at 12:53:22AM +0000, Bobby Eshleman wrote:
>> > On Wed, Aug 02, 2023 at 10:24:44PM +0000, Bobby Eshleman wrote:
>> > > On Sun, Jul 23, 2023 at 12:53:15AM +0300, Arseniy Krasnov
wrote:
>> > > >
>> > > >
>> > > > On 19.07.2023 03:50, Bobby Eshleman wrote:
>> > > > > This patch adds support for multi-transport
datagrams.
>> > > > >
>> > > > > This includes:
>> > > > > - Per-packet lookup of transports when using
sendto(sockaddr_vm)
>> > > > > - Selecting H2G or G2H transport using
VMADDR_FLAG_TO_HOST and CID in
>> > > > >   sockaddr_vm
>> > > > > - rename VSOCK_TRANSPORT_F_DGRAM to
VSOCK_TRANSPORT_F_DGRAM_FALLBACK
>> > > > > - connect() now assigns the transport for (similar
to connectible
>> > > > >   sockets)
>> > > > >
>> > > > > To preserve backwards compatibility with VMCI, some
important changes
>> > > > > are made. The "transport_dgram" /
VSOCK_TRANSPORT_F_DGRAM is changed to
>> > > > > be used for dgrams only if there is not yet a g2h
or h2g transport that
>> > > > > has been registered that can transmit the packet.
If there is a g2h/h2g
>> > > > > transport for that remote address, then that
transport will be used and
>> > > > > not "transport_dgram". This essentially
makes "transport_dgram" a
>> > > > > fallback transport for when h2g/g2h has not yet
gone online, and so it
>> > > > > is renamed "transport_dgram_fallback".
VMCI implements this transport.
>> > > > >
>> > > > > The logic around "transport_dgram" needs
to be retained to prevent
>> > > > > breaking VMCI:
>> > > > >
>> > > > > 1) VMCI datagrams existed prior to h2g/g2h and so
operate under a
>> > > > >    different paradigm. When the vmci transport
comes online, it registers
>> > > > >    itself with the DGRAM feature, but not H2G/G2H.
Only later when the
>> > > > >    transport has more information about its
environment does it register
>> > > > >    H2G or G2H.  In the case that a datagram socket
is created after
>> > > > >    VSOCK_TRANSPORT_F_DGRAM registration but before
G2H/H2G registration,
>> > > > >    the "transport_dgram" transport is the
only registered transport and so
>> > > > >    needs to be used.
>> > > > >
>> > > > > 2) VMCI seems to require a special message be sent
by the transport when a
>> > > > >    datagram socket calls bind(). Under the h2g/g2h
model, the transport
>> > > > >    is selected using the remote_addr which is set
by connect(). At
>> > > > >    bind time there is no remote_addr because often
no connect() has been
>> > > > >    called yet: the transport is null. Therefore,
with a null transport
>> > > > >    there doesn't seem to be any good way for a
datagram socket to tell the
>> > > > >    VMCI transport that it has just had bind()
called upon it.
>> > > > >
>> > > > > With the new fallback logic, after H2G/G2H comes
online the socket layer
>> > > > > will access the VMCI transport via
transport_{h2g,g2h}. Prior to H2G/G2H
>> > > > > coming online, the socket layer will access the
VMCI transport via
>> > > > > "transport_dgram_fallback".
>> > > > >
>> > > > > Only transports with a special datagram fallback
use-case such as VMCI
>> > > > > need to register VSOCK_TRANSPORT_F_DGRAM_FALLBACK.
>> > > > >
>> > > > > Signed-off-by: Bobby Eshleman <bobby.eshleman at
bytedance.com>
>> > > > > ---
>> > > > >  drivers/vhost/vsock.c                   |  1 -
>> > > > >  include/linux/virtio_vsock.h            |  2 --
>> > > > >  include/net/af_vsock.h                  | 10
+++---
>> > > > >  net/vmw_vsock/af_vsock.c                | 64
++++++++++++++++++++++++++-------
>> > > > >  net/vmw_vsock/hyperv_transport.c        |  6 ----
>> > > > >  net/vmw_vsock/virtio_transport.c        |  1 -
>> > > > >  net/vmw_vsock/virtio_transport_common.c |  7 ----
>> > > > >  net/vmw_vsock/vmci_transport.c          |  2 +-
>> > > > >  net/vmw_vsock/vsock_loopback.c          |  1 -
>> > > > >  9 files changed, 58 insertions(+), 36 deletions(-)
>> > > > >
>> > > > > diff --git a/drivers/vhost/vsock.c
b/drivers/vhost/vsock.c
>> > > > > index ae8891598a48..d5d6a3c3f273 100644
>> > > > > --- a/drivers/vhost/vsock.c
>> > > > > +++ b/drivers/vhost/vsock.c
>> > > > > @@ -410,7 +410,6 @@ static struct virtio_transport
vhost_transport = {
>> > > > >  		.cancel_pkt               =
vhost_transport_cancel_pkt,
>> > > > >
>> > > > >  		.dgram_enqueue            =
virtio_transport_dgram_enqueue,
>> > > > > -		.dgram_bind               =
virtio_transport_dgram_bind,
>> > > > >  		.dgram_allow              =
virtio_transport_dgram_allow,
>> > > > >
>> > > > >  		.stream_enqueue           =
virtio_transport_stream_enqueue,
>> > > > > diff --git a/include/linux/virtio_vsock.h
b/include/linux/virtio_vsock.h
>> > > > > index 18cbe8d37fca..7632552bee58 100644
>> > > > > --- a/include/linux/virtio_vsock.h
>> > > > > +++ b/include/linux/virtio_vsock.h
>> > > > > @@ -211,8 +211,6 @@ void
virtio_transport_notify_buffer_size(struct vsock_sock *vsk, u64 *val);
>> > > > >  u64 virtio_transport_stream_rcvhiwat(struct
vsock_sock *vsk);
>> > > > >  bool virtio_transport_stream_is_active(struct
vsock_sock *vsk);
>> > > > >  bool virtio_transport_stream_allow(u32 cid, u32
port);
>> > > > > -int virtio_transport_dgram_bind(struct vsock_sock
*vsk,
>> > > > > -				struct sockaddr_vm *addr);
>> > > > >  bool virtio_transport_dgram_allow(u32 cid, u32
port);
>> > > > >
>> > > > >  int virtio_transport_connect(struct vsock_sock
*vsk);
>> > > > > diff --git a/include/net/af_vsock.h
b/include/net/af_vsock.h
>> > > > > index 305d57502e89..f6a0ca9d7c3e 100644
>> > > > > --- a/include/net/af_vsock.h
>> > > > > +++ b/include/net/af_vsock.h
>> > > > > @@ -96,13 +96,13 @@ struct
vsock_transport_send_notify_data {
>> > > > >
>> > > > >  /* Transport features flags */
>> > > > >  /* Transport provides host->guest communication
*/
>> > > > > -#define VSOCK_TRANSPORT_F_H2G		0x00000001
>> > > > > +#define VSOCK_TRANSPORT_F_H2G			0x00000001
>> > > > >  /* Transport provides guest->host communication
*/
>> > > > > -#define VSOCK_TRANSPORT_F_G2H		0x00000002
>> > > > > -/* Transport provides DGRAM communication */
>> > > > > -#define VSOCK_TRANSPORT_F_DGRAM		0x00000004
>> > > > > +#define VSOCK_TRANSPORT_F_G2H			0x00000002
>> > > > > +/* Transport provides fallback for DGRAM
communication */
>> > > > > +#define VSOCK_TRANSPORT_F_DGRAM_FALLBACK
0x00000004
>> > > > >  /* Transport provides local (loopback)
communication */
>> > > > > -#define VSOCK_TRANSPORT_F_LOCAL		0x00000008
>> > > > > +#define VSOCK_TRANSPORT_F_LOCAL			0x00000008
>> > > > >
>> > > > >  struct vsock_transport {
>> > > > >  	struct module *module;
>> > > > > diff --git a/net/vmw_vsock/af_vsock.c
b/net/vmw_vsock/af_vsock.c
>> > > > > index ae5ac5531d96..26c97b33d55a 100644
>> > > > > --- a/net/vmw_vsock/af_vsock.c
>> > > > > +++ b/net/vmw_vsock/af_vsock.c
>> > > > > @@ -139,8 +139,8 @@ struct proto vsock_proto = {
>> > > > >  static const struct vsock_transport
*transport_h2g;
>> > > > >  /* Transport used for guest->host communication
*/
>> > > > >  static const struct vsock_transport
*transport_g2h;
>> > > > > -/* Transport used for DGRAM communication */
>> > > > > -static const struct vsock_transport
*transport_dgram;
>> > > > > +/* Transport used as a fallback for DGRAM
communication */
>> > > > > +static const struct vsock_transport
*transport_dgram_fallback;
>> > > > >  /* Transport used for local communication */
>> > > > >  static const struct vsock_transport
*transport_local;
>> > > > >  static DEFINE_MUTEX(vsock_register_mutex);
>> > > > > @@ -439,6 +439,18 @@
vsock_connectible_lookup_transport(unsigned int cid, __u8 flags)
>> > > > >  	return transport;
>> > > > >  }
>> > > > >
>> > > > > +static const struct vsock_transport *
>> > > > > +vsock_dgram_lookup_transport(unsigned int cid,
__u8 flags)
>> > > > > +{
>> > > > > +	const struct vsock_transport *transport;
>> > > > > +
>> > > > > +	transport =
vsock_connectible_lookup_transport(cid, flags);
>> > > > > +	if (transport)
>> > > > > +		return transport;
>> > > > > +
>> > > > > +	return transport_dgram_fallback;
>> > > > > +}
>> > > > > +
>> > > > >  /* Assign a transport to a socket and call the
.init transport callback.
>> > > > >   *
>> > > > >   * Note: for connection oriented socket this must
be called when vsk->remote_addr
>> > > > > @@ -475,7 +487,8 @@ int
vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
>> > > > >
>> > > > >  	switch (sk->sk_type) {
>> > > > >  	case SOCK_DGRAM:
>> > > > > -		new_transport = transport_dgram;
>> > > > > +		new_transport =
vsock_dgram_lookup_transport(remote_cid,
>> > > > > +							     remote_flags);
>> > > >
>> > > > I'm a little bit confused about this:
>> > > > 1) Let's create SOCK_DGRAM socket using
vsock_create()
>> > > > 2) for SOCK_DGRAM it calls
'vsock_assign_transport()' and we go here, remote_cid == -1
>> > > > 3) I guess 'vsock_dgram_lookup_transport()'
calls logic from 0002 and returns h2g for such remote cid, which is not
>> > > >    correct I think...
>> > > >
>> > > > Please correct me if i'm wrong
>> > > >
>> > > > Thanks, Arseniy
>> > > >
>> > >
>> > > As I understand, for the VMCI case, if transport_h2g != NULL,
then
>> > > transport_h2g == transport_dgram_fallback. In either case,
>> > > vsk->transport == transport_dgram_fallback.
>> > >
>> > > For the virtio/vhost case, temporarily vsk->transport ==
transport_h2g,
>> > > but it is unused because vsk->transport->dgram_bind ==
NULL.
>> > >
>> > > Until SS_CONNECTED is set by connect() and vsk->transport
is set
>> > > correctly, the send path is barred from using the bad
transport.
>> > >
>> > > I guess the recvmsg() path is a little more sketchy, and
probably only
>> > > works in my test cases because h2g/g2h in the vhost/virtio
case have
>> > > identical dgram_addr_init() implementations.
>> > >
>> > > I think a cleaner solution is maybe checking in
vsock_create() if
>> > > dgram_bind is implemented. If it is not, then
vsk->transport should be
>> > > reset to NULL and a comment added explaining why VMCI
requires this.
>> > >
>> > > Then the other calls can begin explicitly checking for
vsk->transport =>> > > NULL.
>> >
>> > Actually, on further reflection here, in order for the
vsk->transport to
>> > be called in time for ->dgram_addr_init(), it is going to be
necessary
>> > to call vsock_assign_transport() in vsock_dgram_bind() anyway.
>> >
>> > I think this means that the vsock_assign_transport() call can be
removed
>> > from vsock_create() call entirely, and yet VMCI can still dispatch
>> > messages upon bind() calls as needed.
>> >
>> > This would then simplify the whole arrangement, if there
aren't other
>> > unseen issues.
>>
>> This sounds like a good approach.
>>
>> My only question is whether vsock_dgram_bind() is always called for
each
>> dgram socket.
>>
>
>No, not yet.
>
>Currently, receivers may use vsock_dgram_recvmsg() prior to any bind,
>but this should probably change.
>
>For UDP, if we initialize a socket and call recvmsg() with no prior
>bind, then the socket will be auto-bound to 0.0.0.0. I guess vsock
>should probably also auto-bind in this case.
I see.
>
>For other cases, bind may not be called prior to calls to vsock_poll() /
>vsock_getname() (even if it doesn't make sense to do so), but I think it
>is okay as long as vsk->transport is not used.
Makes sense.
>
>vsock_dgram_sendmsg() always auto-binds if needed.
Okay, but the transport for sending messages, doesn't depend on the
local address, right?

Thanks,
Stefano