Paolo Abeni
2023-Aug-01 13:34 UTC
[PATCH net-next v5 4/4] vsock/virtio: MSG_ZEROCOPY flag support
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; > + } > +}[...]> @@ -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