Re: [PATCH net-next v5 4/4] vsock/virtio: MSG_ZEROCOPY flag support
From: Paolo Abeni <pabeni@redhat.com>
Date: 2023-08-01 13:35:07
Also in:
kvm, lkml, virtualization
On Sun, 2023-07-30 at 11:59 +0300, Arseniy Krasnov wrote:
+static int virtio_transport_fill_skb(struct sk_buff *skb,
+ struct virtio_vsock_pkt_info *info,
+ size_t len,
+ bool zcopy)
+{
+ if (zcopy) {
+ return __zerocopy_sg_from_iter(info->msg, NULL, skb,
+ &info->msg->msg_iter,
+ len);
+ } else {No need for an else statement after 'return'
+ void *payload; + int err; + + payload = skb_put(skb, len); + err = memcpy_from_msg(payload, info->msg, len); + if (err) + return -1; + + if (msg_data_left(info->msg)) + return 0; +
This path does not update truesize, evem if it increases the skb len...
+ return 0; + } +}
[...]
quoted hunk ↗ jump to hunk
@@ -214,6 +251,70 @@ static u16 virtio_transport_get_type(struct sock *sk) return VIRTIO_VSOCK_TYPE_SEQPACKET; } +static struct sk_buff *virtio_transport_alloc_skb(struct vsock_sock *vsk, + struct virtio_vsock_pkt_info *info, + size_t payload_len, + bool zcopy, + u32 src_cid, + u32 src_port, + u32 dst_cid, + u32 dst_port) +{ + struct sk_buff *skb; + size_t skb_len; + + skb_len = VIRTIO_VSOCK_SKB_HEADROOM; + + if (!zcopy) + skb_len += payload_len; + + skb = virtio_vsock_alloc_skb(skb_len, GFP_KERNEL); + if (!skb) + return NULL; + + virtio_transport_init_hdr(skb, info, src_cid, src_port, + dst_cid, dst_port, + payload_len); + + /* Set owner here, because '__zerocopy_sg_from_iter()' uses + * owner of skb without check to update 'sk_wmem_alloc'. + */ + if (vsk) + skb_set_owner_w(skb, sk_vsock(vsk));
... which can lead to bad things(TM) if the skb goes trough some later non trivial processing, due to the above skb_set_owner_w(). Additionally can be the following condition be true: vsk == NULL && (info->msg && payload_len > 0) && zcopy ??? If so it looks like skb can go through __zerocopy_sg_from_iter() even without a prior skb_set_owner_w()... Cheers, Paolo