[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8a7772a50a16fbbcb82fc0c5e09f9e31f3427e3d.camel@redhat.com>
Date: Tue, 01 Aug 2023 15:34:07 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Arseniy Krasnov <AVKrasnov@...rdevices.ru>,
Stefan Hajnoczi <stefanha@...hat.com>,
Stefano Garzarella <sgarzare@...hat.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
"Michael S. Tsirkin" <mst@...hat.com>,
Jason Wang <jasowang@...hat.com>,
Bobby Eshleman <bobby.eshleman@...edance.com>
Cc: kvm@...r.kernel.org, virtualization@...ts.linux-foundation.org,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
kernel@...rdevices.ru, oxffffaa@...il.com
Subject: Re: [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
Powered by blists - more mailing lists