+ Dan Carpenter
On Wed, May 31, 2023 at 12:35:10AM +0000, Bobby Eshleman wrote:
> This commit adds support for datagrams over virtio/vsock.
>
> Message boundaries are preserved on a per-skb and per-vq entry basis.
> Messages are copied in whole from the user to an SKB, which in turn is
> added to the scatterlist for the virtqueue in whole for the device.
> Messages do not straddle skbs and they do not straddle packets.
> Messages may be truncated by the receiving user if their buffer is
> shorter than the message.
>
> Other properties of vsock datagrams:
> - Datagrams self-throttle at the per-socket sk_sndbuf threshold.
> - The same virtqueue is used as is used for streams and seqpacket flows
> - Credits are not used for datagrams
> - Packets are dropped silently by the device, which means the virtqueue
> will still get kicked even during high packet loss, so long as the
> socket does not exceed sk_sndbuf.
>
> Future work might include finding a way to reduce the virtqueue kick
> rate for datagram flows with high packet loss.
>
> Signed-off-by: Bobby Eshleman <bobby.eshleman@xxxxxxxxxxxxx>
...
Hi Bobby,
some feedback from my side.
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
...
> @@ -730,11 +754,18 @@ int vsock_bind_stream(struct vsock_sock *vsk,
> }
> EXPORT_SYMBOL(vsock_bind_stream);
>
> -static int __vsock_bind_dgram(struct vsock_sock *vsk,
> - struct sockaddr_vm *addr)
> +static int vsock_bind_dgram(struct vsock_sock *vsk,
> + struct sockaddr_vm *addr)
> {
> - if (!vsk->transport || !vsk->transport->dgram_bind)
> - return -EINVAL;
> + if (!vsk->transport || !vsk->transport->dgram_bind) {
> + int retval;
nit: blank line here
> + spin_lock_bh(&vsock_dgram_table_lock);
> + retval = vsock_bind_common(vsk, addr, vsock_dgram_bind_table,
> + VSOCK_HASH_SIZE);
> + spin_unlock_bh(&vsock_dgram_table_lock);
> +
> + return retval;
> + }
>
> return vsk->transport->dgram_bind(vsk, addr);
> }
...
> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
...
> @@ -47,7 +76,8 @@ virtio_transport_alloc_skb(struct virtio_vsock_pkt_info *info,
> u32 src_cid,
> u32 src_port,
> u32 dst_cid,
> - u32 dst_port)
> + u32 dst_port,
> + int *errp)
> {
> const size_t skb_len = VIRTIO_VSOCK_SKB_HEADROOM + len;
> struct virtio_vsock_hdr *hdr;
> @@ -55,9 +85,21 @@ virtio_transport_alloc_skb(struct virtio_vsock_pkt_info *info,
> void *payload;
> int err;
>
> - skb = virtio_vsock_alloc_skb(skb_len, GFP_KERNEL);
> - if (!skb)
> + /* dgrams do not use credits, self-throttle according to sk_sndbuf
> + * using sock_alloc_send_skb. This helps avoid triggering the OOM.
> + */
> + if (info->vsk && info->type == VIRTIO_VSOCK_TYPE_DGRAM) {
> + skb = virtio_transport_sock_alloc_send_skb(info, skb_len, GFP_KERNEL, &err);
> + } else {
> + skb = virtio_vsock_alloc_skb(skb_len, GFP_KERNEL);
> + if (!skb)
> + err = -ENOMEM;
> + }
> +
> + if (!skb) {
> + *errp = err;
> return NULL;
> + }
>
> hdr = virtio_vsock_hdr(skb);
> hdr->type = cpu_to_le16(info->type);
> @@ -102,6 +144,7 @@ virtio_transport_alloc_skb(struct virtio_vsock_pkt_info *info,
Smatch that err may not be initialised in the out label below.
Just above this context the following appears:
if (info->vsk && !skb_set_owner_sk_safe(skb, sk_vsock(info->vsk))) {
WARN_ONCE(1, "failed to allocate skb on vsock socket with sk_refcnt == 0\n");
goto out;
}
So I wonder if in that case err may not be initialised.
> return skb;
>
> out:
> + *errp = err;
> kfree_skb(skb);
> return NULL;
> }
...
