netdev - Re: [PATCH v5] net/packet: support mergeable feature of virtio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <641dd26c30a9b_f8c982085a@willemb.c.googlers.com.notmuch>
Date:   Fri, 24 Mar 2023 12:40:12 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     沈安琪(凛玥) <amy.saq@...group.com>,
        Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        netdev@...r.kernel.org
Cc:     mst@...hat.com, davem@...emloft.net, jasowang@...hat.com,
        谈鉴锋 <henry.tjf@...group.com>
Subject: Re: [PATCH v5] net/packet: support mergeable feature of virtio

沈安琪(凛玥) wrote:
> 
> 在 2023/3/17 下午10:09, Willem de Bruijn 写道:
> > 沈安琪(凛玥) wrote:
> >> From: Jianfeng Tan <henry.tjf@...group.com>
> >>
> >> Packet sockets, like tap, can be used as the backend for kernel vhost.
> >> In packet sockets, virtio net header size is currently hardcoded to be
> >> the size of struct virtio_net_hdr, which is 10 bytes; however, it is not
> >> always the case: some virtio features, such as mrg_rxbuf, need virtio
> >> net header to be 12-byte long.
> >>
> >> Mergeable buffers, as a virtio feature, is worthy of supporting: packets
> >> that are larger than one-mbuf size will be dropped in vhost worker's
> >> handle_rx if mrg_rxbuf feature is not used, but large packets
> >> cannot be avoided and increasing mbuf's size is not economical.
> >>
> >> With this virtio feature enabled by virtio-user, packet sockets with
> >> hardcoded 10-byte virtio net header will parse mac head incorrectly in
> >> packet_snd by taking the last two bytes of virtio net header as part of
> >> mac header.
> >> This incorrect mac header parsing will cause packet to be dropped due to
> >> invalid ether head checking in later under-layer device packet receiving.
> >>
> >> By adding extra field vnet_hdr_sz with utilizing holes in struct
> >> packet_sock to record currently used virtio net header size and supporting
> >> extra sockopt PACKET_VNET_HDR_SZ to set specified vnet_hdr_sz, packet
> >> sockets can know the exact length of virtio net header that virtio user
> >> gives.
> >> In packet_snd, tpacket_snd and packet_recvmsg, instead of using
> >> hardcoded virtio net header size, it can get the exact vnet_hdr_sz from
> >> corresponding packet_sock, and parse mac header correctly based on this
> >> information to avoid the packets being mistakenly dropped.
> >>
> >> 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>
> > Another patch set was just merged that this will have merge conflicts
> > with. Please respin.
> >
> > https://lore.kernel.org/netdev/20230316011014.992179-4-edumazet@google.com/T/
> 
> 
> Sure thing. We are going to rebase it. The recently-merged patch 
> compacted all bit flags into one long flags field and getting the bit 
> information through the helper function.
> 
> Since our patch removes has_vnet_hdr bit and uses vnet_hdr_sz, which 
> cannot fit in one bit, to indicate whether the packet sock has vnet 
> header or not, we plan to remove PACKET_SOCK_HAS_VNET_HDR from 
> packet_sock_flags and keep the u8 field vnet_hdr_sz in struct 
> packet_sock. After modification, the packet_sock struct will be following:
> 
> @@ -119,9 +119,9 @@ struct packet_sock {
>   	unsigned long		flags;
>   	int			ifindex;	/* bound device		*/
> +	u8			vnet_hdr_sz;
>   	__be16			num;
> 
> 
> I wonder whether this rebase plan looks appropriate for you and am 
> looking forward to your advice here.
> 
> If we are good on the rebasing plan, we will soon send out next version 
> with conflicts resolved. :) Thanks a lot.

Makes sense to me.