Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <43991690-2b53-8211-8aad-693ae5c725e4@sberdevices.ru>
Date:   Tue, 1 Aug 2023 17:06:56 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH net-next v5 4/4] vsock/virtio: MSG_ZEROCOPY flag support
Content-Language: en-US
To:     Paolo Abeni <pabeni@redhat.com>,
        Stefan Hajnoczi <stefanha@redhat.com>,
        Stefano Garzarella <sgarzare@redhat.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        "Michael S. Tsirkin" <mst@redhat.com>,
        Jason Wang <jasowang@redhat.com>,
        Bobby Eshleman <bobby.eshleman@bytedance.com>
CC:     <kvm@vger.kernel.org>, <virtualization@lists.linux-foundation.org>,
        <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <kernel@sberdevices.ru>, <oxffffaa@gmail.com>
References: <20230730085905.3420811-1-AVKrasnov@sberdevices.ru>
 <20230730085905.3420811-5-AVKrasnov@sberdevices.ru>
 <8a7772a50a16fbbcb82fc0c5e09f9e31f3427e3d.camel@redhat.com>
 <1c9f9851-2228-c92b-ce3d-6a84d44e6628@sberdevices.ru>
 <00f2b7bdb18e0eaa42f0cca542a9530564615475.camel@redhat.com>
From:   Arseniy Krasnov <avkrasnov@sberdevices.ru>
In-Reply-To: <00f2b7bdb18e0eaa42f0cca542a9530564615475.camel@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk


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
>