Message-ID: <5c53cfa0-6728-055d-7aa5-6969844d26bf@antgroup.com>
Date:   Wed, 22 Feb 2023 19:43:43 +0800
From:   "沈安琪(凛玥)" <amy.saq@...group.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>
Cc:     "Willem de Bruijn" <willemdebruijn.kernel@...il.com>,
        <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/22 下午7:37, Michael S. Tsirkin 写道:
> On Wed, Feb 22, 2023 at 04:04:34PM +0800, 沈安琪(凛玥) wrote:
>> 在 2023/2/21 下午11:03, Willem de Bruijn 写道:
>>> 沈安琪(凛玥) wrote:
>>>> 在 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.
>>> You're right. This is configured from userspace before the FD is passed
>>> to vhost-net, so indeed this will require packet socket UAPI support.
>>
>> Thanks for quick reply. We will go with adding an extra UAPI here then.
>>
>>
>> Another discussion for designing this UAPI is, whether it will be better to
>> support setting only vnet header size, just like what TAP does in its ioctl,
>> or to support setting a virtio feature bit-map.
>>
>>
>> UAPI setting only vnet header size
>>
>> Pros:
>>
>> 1. It aligns with how other virito backend devices communicate with
>> virtio-user
>>
>> 2. We can use the holes in struct packet_socket (net/packet/internal.h:120)
>> to record the extra information since the size info only takes 8 bits.
>>
>> Cons:
>>
>> 1. It may have more information that virtio-user needs to communicate with
>> packet socket in the future and needs to add more UAPI supports here.
>>
>> To Michael: Is there any other information that backend device needs and
>> will be given from virtio-user?
>
> Yes e.g. I already mentioned virtio 1.0 wrt LE versus native endian
> format.
>
>
>> UAPI setting a virtio feature bit-map
>>
>> Pros:
>>
>> 1. It is more general and may reduce future UAPI changes.
>>
>> Cons:
>>
>> 1. A virtio feature bit-map needs 64 bits, which needs to add an extra field
>> in packet_sock struct
>>
>> 2. Virtio-user needs to aware that using packet socket as backend supports
>> different approach to negotiate the vnet header size.
>>
>>
>> We really appreciate any suggestion or discussion on this design choice of
>> UAPI.
> In the end it's ok with just size too, you just probably shouldn't say
> you support VERSION_1 if you are not passing that bit.
>

Sorry for the confusion here that we mentioned VERSION_1 in the commit 
log. We actually just attended to give an example of what features that 
may need 12-byte vnet header. We will remove it from the commit log in 
patch v2 to avoid confusion here. Thanks a lot for your suggestions.

Powered by blists - more mailing lists