On 01.08.2023 17:04, Paolo Abeni wrote:
> On Tue, 2023-08-01 at 16:36 +0300, Arseniy Krasnov wrote:
>>
>> On 01.08.2023 16:34, Paolo Abeni wrote:
>>> 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...
>>
>> Thanks, I'll fix it.
>>
>>>
>>>> + 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
>>>
>>> ???
>>
>> No, vsk == NULL only when we reset connection, in that case both info->msg == NULL and payload_len == 0,
>> as this is control message without any data.
>
> Perhaps a comment with possibly even a WARN_ON_ONCE(!<the above>) could
> help ;)
Ack
Thanks, Arseniy
>
> Thanks!
>
> Paolo
>
