Message-ID: <a737c617-6722-7002-1ead-4c5bed452595@antgroup.com>
Date:   Tue, 21 Feb 2023 17:40:14 +0800
From:   "沈安琪(凛玥)" <amy.saq@...group.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        "Michael S. Tsirkin" <mst@...hat.com>
Cc:     <netdev@...r.kernel.org>, <davem@...emloft.net>,
        <jasowang@...hat.com>,
        "谈鉴锋" <henry.tjf@...group.com>
Subject: Re: [PATCH 2/2] net/packet: send and receive pkt with given
 vnet_hdr_sz


在 2023/2/14 下午10:28, Willem de Bruijn 写道:
> 沈安琪(凛玥) wrote:
>> 在 2023/2/10 下午11:39, Willem de Bruijn 写道:
>>> Michael S. Tsirkin wrote:
>>>> On Fri, Feb 10, 2023 at 12:01:03PM +0800, 沈安琪(凛玥) wrote:
>>>>> 在 2023/2/9 下午9:07, Michael S. Tsirkin 写道:
>>>>>> On Thu, Feb 09, 2023 at 08:43:15PM +0800, 沈安琪(凛玥) wrote:
>>>>>>> From: "Jianfeng Tan" <henry.tjf@...group.com>
>>>>>>>
>>>>>>> When raw socket is used as the backend for kernel vhost, currently it
>>>>>>> will regard the virtio net header as 10-byte, which is not always the
>>>>>>> case since some virtio features need virtio net header other than
>>>>>>> 10-byte, such as mrg_rxbuf and VERSION_1 that both need 12-byte virtio
>>>>>>> net header.
>>>>>>>
>>>>>>> Instead of hardcoding virtio net header length to 10 bytes, tpacket_snd,
>>>>>>> tpacket_rcv, packet_snd and packet_recvmsg now get the virtio net header
>>>>>>> size that is recorded in packet_sock to indicate the exact virtio net
>>>>>>> header size that virtio user actually prepares in the packets. By doing
>>>>>>> so, it can fix the issue of incorrect mac header parsing when these
>>>>>>> virtio features that need virtio net header other than 10-byte are
>>>>>>> enable.
>>>>>>>
>>>>>>> Signed-off-by: Jianfeng Tan <henry.tjf@...group.com>
>>>>>>> Co-developed-by: Anqi Shen <amy.saq@...group.com>
>>>>>>> Signed-off-by: Anqi Shen <amy.saq@...group.com>
>>>>>> Does it handle VERSION_1 though? That one is also LE.
>>>>>> Would it be better to pass a features bitmap instead?
>>>>> Thanks for quick reply!
>>>>>
>>>>> I am a little confused abot what "LE" presents here?
>>>> LE == little_endian.
>>>> Little endian format.
>>>>
>>>>> For passing a features bitmap to af_packet here, our consideration is
>>>>> whether it will be too complicated for af_packet to understand the virtio
>>>>> features bitmap in order to get the vnet header size. For now, all the
>>>>> virtio features stuff is handled by vhost worker and af_packet actually does
>>>>> not need to know much about virtio features. Would it be better if we keep
>>>>> the virtio feature stuff in user-level and let user-level tell af_packet how
>>>>> much space it should reserve?
>>>> Presumably, we'd add an API in include/linux/virtio_net.h ?
>>> Better leave this opaque to packet sockets if they won't act on this
>>> type info.
>>>    
>>> This patch series probably should be a single patch btw. As else the
>>> socket option introduced in the first is broken at that commit, since
>>> the behavior is only introduced in patch 2.
>>
>> Good point, will merge this patch series into one patch.
>>
>>
>> Thanks for Michael's enlightening advice, we plan to modify current UAPI
>> change of adding an extra socketopt from only setting vnet header size
>> only to setting a bit-map of virtio features, and implement another
>> helper function in include/linux/virtio_net.h to parse the feature
>> bit-map. In this case, packet sockets have no need to understand the
>> feature bit-map but only pass this bit-map to virtio_net helper and get
>> back the information, such as vnet header size, it needs.
>>
>> This change will make the new UAPI more general and avoid further
>> modification if there are more virtio features to support in the future.
>>
> Please also comment how these UAPI extension are intended to be used.
> As that use is not included in this initial patch series.
>
> If the only intended user is vhost-net, we can consider not exposing
> outside the kernel at all. That makes it easier to iterate if
> necessary (no stable ABI) and avoids accidentally opening up new
> avenues for bugs and exploits (syzkaller has a history with
> virtio_net_header options).


Our concern is, it seems there is no other solution than uapi to let 
packet sockets know the vnet header size they should use.

Receiving packets in vhost driver, implemented in drivers/vhost/net.c: 
1109 handle_rx(), will abstract the backend device it uses and directly 
invoke the corresponding socket ops with no extra information indicating 
it is invoked by vhost worker. Vhost worker actually does not know the 
type of backend device it is using; only virito-user knows what type of 
backend device it uses. Therefore, it seems impossible to let vhost set 
the vnet header information to the target backend device.

Tap, another kind of backend device vhost may use, lets virtio-user set 
whether it needs vnet header and how long the vnet header is through 
ioctl. (implemented in drivers/net/tap.c:1066)

In this case, we wonder whether we should align with what tap does and 
set vnet hdr size through setsockopt for packet_sockets.

We really appreciate suggestions on if any, potential approachs to pass 
this vnet header size information from virtio-user to packet-socket.

Powered by blists - more mailing lists