On Tue, Jul 23, 2024 at 7:42 AM Stefano Garzarella [off-list ref] wrote:

On Wed, Jul 10, 2024 at 09:25:50PM GMT, Amery Hung wrote:

quoted

From: Bobby Eshleman <redacted>

This commit adds the common datagram receive functionality for virtio
transports. It does not add the vhost/virtio users of that
functionality.

This functionality includes:
- changes to the virtio_transport_recv_pkt() path for finding the
 bound socket receiver for incoming packets
- virtio_transport_recv_pkt() saves the source cid and port to the
 control buffer for recvmsg() to initialize sockaddr_vm structure
 when using datagram

Signed-off-by: Bobby Eshleman <redacted>
Signed-off-by: Amery Hung <redacted>
---
net/vmw_vsock/virtio_transport_common.c | 79 +++++++++++++++++++++----
1 file changed, 66 insertions(+), 13 deletions(-)

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 46cd1807f8e3..a571b575fde9 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c

@@ -235,7 +235,9 @@ EXPORT_SYMBOL_GPL(virtio_transport_deliver_tap_pkt);

static u16 virtio_transport_get_type(struct sock *sk)
{
-      if (sk->sk_type == SOCK_STREAM)
+      if (sk->sk_type == SOCK_DGRAM)
+              return VIRTIO_VSOCK_TYPE_DGRAM;
+      else if (sk->sk_type == SOCK_STREAM)
              return VIRTIO_VSOCK_TYPE_STREAM;
      else
              return VIRTIO_VSOCK_TYPE_SEQPACKET;

@@ -1422,6 +1424,33 @@ virtio_transport_recv_enqueue(struct vsock_sock *vsk,
              kfree_skb(skb);

}

+static void
+virtio_transport_dgram_kfree_skb(struct sk_buff *skb, int err)
+{
+      if (err == -ENOMEM)
+              kfree_skb_reason(skb, SKB_DROP_REASON_SOCKET_RCVBUFF);
+      else if (err == -ENOBUFS)
+              kfree_skb_reason(skb, SKB_DROP_REASON_PROTO_MEM);
+      else
+              kfree_skb(skb);
+}
+
+/* This function takes ownership of the skb.
+ *
+ * It either places the skb on the sk_receive_queue or frees it.
+ */
+static void
+virtio_transport_recv_dgram(struct sock *sk, struct sk_buff *skb)
+{
+      int err;
+
+      err = sock_queue_rcv_skb(sk, skb);
+      if (err) {
+              virtio_transport_dgram_kfree_skb(skb, err);
+              return;
+      }
+}
+
static int
virtio_transport_recv_connected(struct sock *sk,
                              struct sk_buff *skb)

@@ -1591,7 +1620,8 @@ virtio_transport_recv_listen(struct sock *sk, struct sk_buff *skb,

static bool virtio_transport_valid_type(u16 type)
{
      return (type == VIRTIO_VSOCK_TYPE_STREAM) ||
-             (type == VIRTIO_VSOCK_TYPE_SEQPACKET);
+             (type == VIRTIO_VSOCK_TYPE_SEQPACKET) ||
+             (type == VIRTIO_VSOCK_TYPE_DGRAM);
}

/* We are under the virtio-vsock's vsock->rx_lock or vhost-vsock's vq->mutex

@@ -1601,44 +1631,57 @@ void virtio_transport_recv_pkt(struct virtio_transport *t,
                             struct sk_buff *skb)

{
      struct virtio_vsock_hdr *hdr = virtio_vsock_hdr(skb);
+      struct vsock_skb_cb *vsock_cb;

This can be defined in the block where it's used.

Got it.

quoted

      struct sockaddr_vm src, dst;
      struct vsock_sock *vsk;
      struct sock *sk;
      bool space_available;
+      u16 type;

      vsock_addr_init(&src, le64_to_cpu(hdr->src_cid),
                      le32_to_cpu(hdr->src_port));
      vsock_addr_init(&dst, le64_to_cpu(hdr->dst_cid),
                      le32_to_cpu(hdr->dst_port));

+      type = le16_to_cpu(hdr->type);
+
      trace_virtio_transport_recv_pkt(src.svm_cid, src.svm_port,
                                      dst.svm_cid, dst.svm_port,
                                      le32_to_cpu(hdr->len),
-                                      le16_to_cpu(hdr->type),
+                                      type,
                                      le16_to_cpu(hdr->op),
                                      le32_to_cpu(hdr->flags),
                                      le32_to_cpu(hdr->buf_alloc),
                                      le32_to_cpu(hdr->fwd_cnt));

-      if (!virtio_transport_valid_type(le16_to_cpu(hdr->type))) {
+      if (!virtio_transport_valid_type(type)) {
              (void)virtio_transport_reset_no_sock(t, skb);
              goto free_pkt;
      }

-      /* The socket must be in connected or bound table
-       * otherwise send reset back
+      /* For stream/seqpacket, the socket must be in connected or bound table
+       * otherwise send reset back.
+       *
+       * For datagrams, no reset is sent back.
       */
      sk = vsock_find_connected_socket(&src, &dst);
      if (!sk) {
-              sk = vsock_find_bound_socket(&dst);
-              if (!sk) {
-                      (void)virtio_transport_reset_no_sock(t, skb);
-                      goto free_pkt;
+              if (type == VIRTIO_VSOCK_TYPE_DGRAM) {
+                      sk = vsock_find_bound_dgram_socket(&dst);
+                      if (!sk)
+                              goto free_pkt;
+              } else {
+                      sk = vsock_find_bound_socket(&dst);
+                      if (!sk) {
+                              (void)virtio_transport_reset_no_sock(t, skb);
+                              goto free_pkt;
+                      }
              }
      }

-      if (virtio_transport_get_type(sk) != le16_to_cpu(hdr->type)) {
-              (void)virtio_transport_reset_no_sock(t, skb);
+      if (virtio_transport_get_type(sk) != type) {
+              if (type != VIRTIO_VSOCK_TYPE_DGRAM)
+                      (void)virtio_transport_reset_no_sock(t, skb);
              sock_put(sk);
              goto free_pkt;
      }

@@ -1654,12 +1697,21 @@ void virtio_transport_recv_pkt(struct virtio_transport *t,

      /* Check if sk has been closed before lock_sock */
      if (sock_flag(sk, SOCK_DONE)) {
-              (void)virtio_transport_reset_no_sock(t, skb);
+              if (type != VIRTIO_VSOCK_TYPE_DGRAM)
+                      (void)virtio_transport_reset_no_sock(t, skb);
              release_sock(sk);
              sock_put(sk);
              goto free_pkt;
      }

+      if (sk->sk_type == SOCK_DGRAM) {
+              vsock_cb = vsock_skb_cb(skb);
+              vsock_cb->src_cid = src.svm_cid;
+              vsock_cb->src_port = src.svm_port;
+              virtio_transport_recv_dgram(sk, skb);

What about adding an API that transports can use to hide this?

I mean something that hide vsock_cb creation and queue packet in the
socket receive queue. I'd also not expose vsock_skb_cb in an header, but
I'd handle it internally in af_vsock.c. So I'd just expose API to
queue/dequeue them.

Got it. I will move vsock_skb_cb to af_vsock.c and create an API:

vsock_dgram_skb_save_src_addr(struct sk_buff *skb, u32 cid, u32 port)

Different dgram implementations will call this API instead of the code
block above to save the source address information into the control
buffer.

A side note on why this is a vsock API instead of a member function in
transport: As we move to support multi-transport dgram, different
transport implementations can place skb into the sk->sk_receive_queue.
Therefore, we cannot call transport-specific function in
vsock_dgram_recvmsg() to initialize struct sockaddr_vm. Hence, the
receiving paths of different transports need to call this API to save
source address.

Also why VMCI is using sk_receive_skb(), while we are using
sock_queue_rcv_skb()?

I _think_ originally we referred to UDP and UDS when designing virtio
dgram, and ended up with placing skb into sk_receive_queue directly. I
will look into this to provide better justification.

Thank you,
Amery

Thanks,
Stefano

quoted

+              goto out;
+      }
+
      space_available = virtio_transport_space_update(sk, skb);

      /* Update CID in case it has changed after a transport reset event */

@@ -1691,6 +1743,7 @@ void virtio_transport_recv_pkt(struct virtio_transport *t,
              break;
      }

+out:
      release_sock(sk);

      /* Release refcnt obtained when we fetched this socket out of the
--

2.20.1