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...
Sorry, but what is potential problem here ? In this path I copy data from the user's
buffer to the linear skb (there is no fragged part in this case). I think 'truesize'
is constant in this case - it is SKB_TRUESIZE(length of skb buffer) - there is no need
to update it as 'truesize' does not show amount of data in skb, only real size of
skb's buffer.
For non-linear case, __zerocopy_sg_from_iter() always updates 'sk_wmem_alloc' of the
socket during iterating over frags array.
Also 'skb_set_owner_w()' is called before this code, thus setting 'sk_wmem_alloc' to the
'truesize' value of the skb.
Thanks, Arseniy
>
>> + 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
>
