Date:   Tue, 21 Feb 2023 10:03:23 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     沈安琪(凛玥) <amy.saq@...group.com>,
        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

沈安琪(凛玥) 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.

Powered by blists - more mailing lists