| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <bpb36dtlbs6osr5cudvwrbagt7bls3cllg35lsusrly5pxwe7o@kjphrbuc64ix>
Date: Tue, 23 Jul 2024 16:42:00 +0200
From: Stefano Garzarella <sgarzare@...hat.com>
To: Amery Hung <ameryhung@...il.com>
Cc: stefanha@...hat.com, mst@...hat.com, jasowang@...hat.com,
xuanzhuo@...ux.alibaba.com, davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, kys@...rosoft.com, haiyangz@...rosoft.com, wei.liu@...nel.org,
decui@...rosoft.com, bryantan@...are.com, vdasa@...are.com, pv-drivers@...are.com,
dan.carpenter@...aro.org, simon.horman@...igine.com, oxffffaa@...il.com,
kvm@...r.kernel.org, virtualization@...ts.linux-foundation.org,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org, linux-hyperv@...r.kernel.org,
bpf@...r.kernel.org, bobby.eshleman@...edance.com, jiang.wang@...edance.com,
amery.hung@...edance.com, xiyou.wangcong@...il.com
Subject: Re: [RFC PATCH net-next v6 07/14] virtio/vsock: add common datagram
send path
On Wed, Jul 10, 2024 at 09:25:48PM GMT, Amery Hung wrote:
>From: Bobby Eshleman <bobby.eshleman@...edance.com>
>
>This commit implements the common function
>virtio_transport_dgram_enqueue for enqueueing datagrams. It does not add
>usage in either vhost or virtio yet.
>
>Signed-off-by: Bobby Eshleman <bobby.eshleman@...edance.com>
>Signed-off-by: Amery Hung <amery.hung@...edance.com>
>---
> include/linux/virtio_vsock.h | 1 +
> include/net/af_vsock.h | 2 +
> net/vmw_vsock/af_vsock.c | 2 +-
> net/vmw_vsock/virtio_transport_common.c | 87 ++++++++++++++++++++++++-
> 4 files changed, 90 insertions(+), 2 deletions(-)
>
>diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
>index f749a066af46..4408749febd2 100644
>--- a/include/linux/virtio_vsock.h
>+++ b/include/linux/virtio_vsock.h
>@@ -152,6 +152,7 @@ struct virtio_vsock_pkt_info {
> u16 op;
> u32 flags;
> bool reply;
>+ u8 remote_flags;
> };
>
> struct virtio_transport {
>diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
>index 44db8f2c507d..6e97d344ac75 100644
>--- a/include/net/af_vsock.h
>+++ b/include/net/af_vsock.h
>@@ -216,6 +216,8 @@ void vsock_for_each_connected_socket(struct vsock_transport *transport,
> void (*fn)(struct sock *sk));
> int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk);
> bool vsock_find_cid(unsigned int cid);
>+const struct vsock_transport *vsock_dgram_lookup_transport(unsigned int cid,
>+ __u8 flags);
Why __u8 and not just u8?
>
> struct vsock_skb_cb {
> unsigned int src_cid;
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index ab08cd81720e..f83b655fdbe9 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -487,7 +487,7 @@ vsock_connectible_lookup_transport(unsigned int cid, __u8 flags)
> return transport;
> }
>
>-static const struct vsock_transport *
>+const struct vsock_transport *
> vsock_dgram_lookup_transport(unsigned int cid, __u8 flags)
> {
> const struct vsock_transport *transport;
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index a1c76836d798..46cd1807f8e3 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -1040,13 +1040,98 @@ int virtio_transport_shutdown(struct vsock_sock *vsk, int mode)
> }
> EXPORT_SYMBOL_GPL(virtio_transport_shutdown);
>
>+static int virtio_transport_dgram_send_pkt_info(struct vsock_sock *vsk,
>+ struct virtio_vsock_pkt_info *info)
>+{
>+ u32 src_cid, src_port, dst_cid, dst_port;
>+ const struct vsock_transport *transport;
>+ const struct virtio_transport *t_ops;
>+ struct sock *sk = sk_vsock(vsk);
>+ struct virtio_vsock_hdr *hdr;
>+ struct sk_buff *skb;
>+ void *payload;
>+ int noblock = 0;
>+ int err;
>+
>+ info->type = virtio_transport_get_type(sk_vsock(vsk));
>+
>+ if (info->pkt_len > VIRTIO_VSOCK_MAX_PKT_BUF_SIZE)
>+ return -EMSGSIZE;
>+
>+ transport = vsock_dgram_lookup_transport(info->remote_cid, info->remote_flags);
Can `transport` be null?
I don't understand why we are calling vsock_dgram_lookup_transport()
again. Didn't we already do that in vsock_dgram_sendmsg()?
Also should we add a comment mentioning that we can't use
virtio_transport_get_ops()? IIUC becuase the vsk can be not assigned
to a specific transport, right?
>+ t_ops = container_of(transport, struct virtio_transport, transport);
>+ if (unlikely(!t_ops))
>+ return -EFAULT;
>+
>+ if (info->msg)
>+ noblock = info->msg->msg_flags & MSG_DONTWAIT;
>+
>+ /* Use sock_alloc_send_skb to throttle by sk_sndbuf. This helps avoid
>+ * triggering the OOM.
>+ */
>+ skb = sock_alloc_send_skb(sk, info->pkt_len + VIRTIO_VSOCK_SKB_HEADROOM,
>+ noblock, &err);
>+ if (!skb)
>+ return err;
>+
>+ skb_reserve(skb, VIRTIO_VSOCK_SKB_HEADROOM);
>+
>+ src_cid = t_ops->transport.get_local_cid();
>+ src_port = vsk->local_addr.svm_port;
>+ dst_cid = info->remote_cid;
>+ dst_port = info->remote_port;
>+
>+ hdr = virtio_vsock_hdr(skb);
>+ hdr->type = cpu_to_le16(info->type);
>+ hdr->op = cpu_to_le16(info->op);
>+ hdr->src_cid = cpu_to_le64(src_cid);
>+ hdr->dst_cid = cpu_to_le64(dst_cid);
>+ hdr->src_port = cpu_to_le32(src_port);
>+ hdr->dst_port = cpu_to_le32(dst_port);
>+ hdr->flags = cpu_to_le32(info->flags);
>+ hdr->len = cpu_to_le32(info->pkt_len);
>+
>+ if (info->msg && info->pkt_len > 0) {
>+ payload = skb_put(skb, info->pkt_len);
>+ err = memcpy_from_msg(payload, info->msg, info->pkt_len);
>+ if (err)
>+ goto out;
>+ }
>+
>+ trace_virtio_transport_alloc_pkt(src_cid, src_port,
>+ dst_cid, dst_port,
>+ info->pkt_len,
>+ info->type,
>+ info->op,
>+ info->flags,
>+ false);
>+
>+ return t_ops->send_pkt(skb);
>+out:
>+ kfree_skb(skb);
>+ return err;
>+}
>+
> int
> virtio_transport_dgram_enqueue(struct vsock_sock *vsk,
> struct sockaddr_vm *remote_addr,
> struct msghdr *msg,
> size_t dgram_len)
> {
>- return -EOPNOTSUPP;
>+ /* Here we are only using the info struct to retain style uniformity
>+ * and to ease future refactoring and merging.
>+ */
>+ struct virtio_vsock_pkt_info info = {
>+ .op = VIRTIO_VSOCK_OP_RW,
>+ .remote_cid = remote_addr->svm_cid,
>+ .remote_port = remote_addr->svm_port,
>+ .remote_flags = remote_addr->svm_flags,
>+ .msg = msg,
>+ .vsk = vsk,
>+ .pkt_len = dgram_len,
>+ };
>+
>+ return virtio_transport_dgram_send_pkt_info(vsk, &info);
> }
> EXPORT_SYMBOL_GPL(virtio_transport_dgram_enqueue);
>
>--
>2.20.1
>
Powered by blists - more mailing lists